-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate confusing option names #1692
Conversation
This reverts commit 524110b.
Codecov Report
@@ Coverage Diff @@
## release/3.3 #1692 +/- ##
===============================================
+ Coverage 23.01% 23.13% +0.11%
===============================================
Files 253 253
Lines 5731 5745 +14
Branches 682 693 +11
===============================================
+ Hits 1319 1329 +10
+ Misses 3921 3918 -3
- Partials 491 498 +7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. (Let's not merge until #1693, please)
lib/ui/src/libs/key_events.js
Outdated
DOWN_PANEL: 2, | ||
LEFT_PANEL: 3, | ||
ADDON_PANEL: 2, | ||
NAVIGATION_PANEL: 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why everywhere it's called "Stories Panel" but here is "Navigation Panel"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, skipped when refactoring
Great! 👍 But I prefer to have a strong "How to test" section and maybe better to merge it right before the next minor releasing |
I've made one, but both cases are covered by added tests |
lib/ui/src/libs/key_events.js
Outdated
@@ -2,14 +2,14 @@ import keycode from 'keycode'; | |||
|
|||
export const features = { | |||
FULLSCREEN: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's a good time to also switch from numbers to literals here, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to update the readme file too. The docs might need an update as well.
https://github.com/storybooks/storybook/blob/master/addons/options/README.md
I find it weird to see docs for options that are not yet actually available |
…ft-down # Conflicts: # lib/ui/src/modules/ui/components/layout/index.test.js
@danielduan I've changed base fork to |
"Left Panel" => "Stories Panel"
"Down Panel" => "Addon Panel"
Old option names stay fully supported for now, but produce a deprecation warning in development
I'll update the addon-options docs as soon as it's published
How to test:
cra-kitchen-sink
and run itsetOptions
fromcra-kitchen-sink
(because of Options set via addon should not override URL params #1690) and addleft=0&down=0
to URL