addon-viewport: Use the new parameterized way of decorators #3610
Conversation
mshaaban088
requested review from
danielduan,
saponifi3d and
usulpro
as code owners
|
|
||
| const subscription = () => { | ||
| const subscription = handler => () => { |
There was a problem hiding this comment.
I noticed an issue in the previous implementation when you create 2 stories with subscription/onViewportChange, the 2nd one receives the event twice every time you change the viewport, which should not be
There was a problem hiding this comment.
Now it creates a new subscription on each rerender (e.g. knob change): subscription(handler) is a new function each time
There was a problem hiding this comment.
Can you please add a second example with onViewportChange so that everyoune could reproduce the issue. Just by looking at the change I can't figure out why it happened
There was a problem hiding this comment.
Oh I see, changing of handler variable prevents the subscription from removing. This should fix it without affecting performance:
const callHandler = (...args) => handler(...args)
const subscription = () => {
const channel = addons.getChannel();
channel.on(VIEWPORT_CHANGED_EVENT_ID, callHandler);
return () => channel.removeListener(VIEWPORT_CHANGED_EVENT_ID, callHandler);
};
There was a problem hiding this comment.
Yep, this should work.
There was a problem hiding this comment.
@Hypnosphi do you still need the second example?
There was a problem hiding this comment.
As you wish. Or maybe a testcase would be better, just to make sure it doesn't break in future
There was a problem hiding this comment.
I agree, test case is even better. Will add it
| const viewportOptions = | ||
| typeof storyOptions === 'string' ? { name: storyOptions } : storyOptions; | ||
|
|
||
| applyViewportOptions(viewportOptions); |
There was a problem hiding this comment.
is it needed when there are neither options nor parameters?
There was a problem hiding this comment.
No, not needed in such case
There was a problem hiding this comment.
Then you can pass skipIfNoParametersOrOptions: true to makeDecorator
|
|
||
| export const Viewport = deprecate(({ children, ...options }) => { | ||
| setViewport(options); | ||
| applyViewportOptions(options); | ||
| return children; | ||
| }, `<Viewport> usage is deprecated, use .addDecorator(withViewport(viewport)) instead`); |
There was a problem hiding this comment.
I think it can be now "use .addParameters({viewport}) instead"
There was a problem hiding this comment.
I updated it.
I wasn't aware of such method to be honest :)
Codecov Report
@@ Coverage Diff @@
## master #3610 +/- ##
==========================================
+ Coverage 40.02% 40.05% +0.03%
==========================================
Files 481 481
Lines 5254 5255 +1
Branches 862 864 +2
==========================================
+ Hits 2103 2105 +2
+ Misses 2634 2632 -2
- Partials 517 518 +1
Continue to review full report at Codecov.
|
What I did
I used the new parameterized way of decorators described in #3559
How to test
yarn test
Is this testable with Jest or Chromatic screenshots?
No
Does this need a new example in the kitchen sink apps?
No, I just updated the already existing one in
official-storybookexamplesDoes this need an update to the documentation?
Yes and added
If your answer is yes to any of these, please make sure to include it in your PR.
For maintainers only: Please tag your pull request with at least one of the following:
["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]