Add params-based API to options addon #3965

tmeasday · 2018-08-06T05:52:16Z

Issue:

Options API wasn’t using params and couldn’t really be used per-story

What I did

Added a params-based API: .add(name, story, { options: {… } })
Added stories to official storybook
Updated .setOptions internally to reset each time, rather than remembering the previous state (and only overriding new keys). This didn’t really make sense and was broken when you switched between stories that set options, unless they all set all options, which users aren’t going to do.

STILL OUTSTANDING

When you change panels via options it does not re-calculate sizing. So if you hide the addons panel, it will leave a bunch of empty space. Can someone who knows the internals give me some advice on how to re-calculate sizes when options are set?

How to test

Is this testable with Jest or Chromatic screenshots?

There are some jest test that touch on it, but the UI is not currently captured by our screenshots for these stories.

Does this need a new example in the kitchen sink apps?

Yes, done.

Does this need an update to the documentation?

Yes, ~~done~~ (TODO).

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"]

storybook-safe-bot · 2018-08-06T06:06:35Z

	Fails
🚫	PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS

ndelangen · 2018-08-06T10:08:48Z

@tmeasday I saw this PR first, looked great to me, but maybe you have thoughts on it?

#3958

You seem to be be of the opinion options should reset. Perhaps you can rebase this PR and implement that, if you so desire?

tmeasday · 2018-08-07T00:53:58Z

Wow I was totally gazumped by @nm123github -- nice ;)

From playing around with it a bit, I think the options need to be reset, otherwise you get super weird behaviour as the options drift as you change stories.

I'll rebase this PR against current master. New questions based on that PR:

~~I'm not sure we should have a story about this in the marko example app? I think these kind SB-wide stories should just be in official storybook, right?~~
~~We should ensure that the global setOptions function actually calls setParameters so the "default" options are the user's. (As pointed out by @nm123github)~~ actually this part works.
We need to deal with panel resizing still, when you toggle between "addon panel open" and "addon panel closed" at any rate.

…tions`

@nm123github

This is just for simplicity. I think it is best if non-framework specific stories all live in official-storybook; that way they are easier to find in the future. cc @nm123github

@nm123github

cc @nm123github -- FYI -- we made this shortcut so you don't need to call `addDecorator` followed by `addParameters`.

nm123github · 2018-08-07T17:59:47Z

@tmeasday had no idea you were working on this too 🙂 , thanks for the review..

- I'm not sure we should have a story about this in the marko example app? I think these kind SB-wide stories should just be in official storybook, right?
Agreed, i will remove those and add something like that in official storybook

- We need to deal with panel resizing still, when you toggle between "addon panel open" and "addon panel closed" at any rate.
Can you please tell me little more about this? Maybe a gify or some screenshots might help. Also, feel free to PM on slack.

From playing around with it a bit, I think the options need to be reset, otherwise you get super weird behaviour as the options drift as you change stories.
^ you know, i would kinda agree with that, but @ndelangen mentioned "setOptions being persistent is expected behavior I think?". Also, i am not sure if there is current functionality that allows to setOptions specifically for a story, or if that will need to be built.

ndelangen · 2018-08-07T22:33:37Z

Let me know when it's ready for review?

tmeasday · 2018-08-08T00:02:07Z

I've done 1. in this PR also. So from my perspective the remaining issue is just resizing the panels when they are hidden. Let me make a couple of gifs.

tmeasday · 2018-08-08T00:30:39Z

"setOptions being persistent is expected behavior I think?". Also, i am not sure if there is current functionality that allows to setOptions specifically for a story, or if that will need to be built.

I think maybe I have confused things a little -- this PR retains top-level setOptions being persisted, what it does not persist is story-level options being persisted. For instance in current master we have this behaviour:

Notice how the Actions story's addons panel location depends on the previous story you looked at. It's even more of a problem when a story is e.g. hiding the panel.

If the user sets options on every story it is not a problem, but this isn't what users will expect i don't think. In any case I think this PR has the best of both worlds (story-level options are not remembered, global options set via setOptions are).

nm123github · 2018-08-08T00:34:23Z

Thanks for taking care of this @tmeasday

tmeasday · 2018-08-08T00:36:13Z

No worries @nm123github -- just standing on your shoulders

tmeasday · 2018-08-08T00:39:12Z

On the subject on panel resizing I cannot reproduce the problem on master so hopefully I can figure out what's different on this branch and solve it. Will look later today.

igor-dv · 2018-08-08T08:40:32Z

@tmeasday, probably I am missing something, but playing with preview of your PR (didn't review the code yet), I do see options are persistent between stories. For example, the "Custom Storybook" name is remaining when navigating out of the story that changed it.

BTW I would also limit what people can change. For example, not allow changing hierarchy options.

codecov · 2018-08-08T09:39:44Z

Codecov Report

Merging #3965 into master will increase coverage by 0.02%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master    #3965      +/-   ##
==========================================
+ Coverage   40.46%   40.48%   +0.02%     
==========================================
  Files         491      491              
  Lines        5835     5832       -3     
  Branches      793      791       -2     
==========================================
  Hits         2361     2361              
+ Misses       3085     3084       -1     
+ Partials      389      387       -2

Impacted Files	Coverage Δ
addons/options/preview.js	`100% <ø> (ø)`	⬆️
lib/ui/src/modules/api/index.js	`0% <ø> (ø)`	⬆️
addons/options/src/preview/index.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js	`40.62% <100%> (ø)`	⬆️
lib/ui/src/modules/shortcuts/defaultState.js	`100% <100%> (ø)`
lib/ui/src/modules/api/actions/api.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7d3a57...d309b91. Read the comment docs.

tmeasday · 2018-08-08T09:54:24Z

@igor-dv

probably I am missing something, but playing with preview of your PR (didn't review the code yet), I do see options are persistent between stories. For example, the "Custom Storybook" name is remaining when navigating out of the story that changed it.

Nope, you are not missing something, I thought I had fixed the problem but it turns out only partially.

Any ideas on how we could fix it properly? Would using a subscription help? I'm aware we don't want options to thrash as you go between stories. Ideally you would have the withOptions decorator applied at all times, but I don't think we can guarantee that.

BTW I would also limit what people can change. For example, not allow changing hierarchy options.

Yeah I think maybe we should just do the shortcutOptions and not the uiOptions

tmeasday · 2018-08-08T10:15:15Z

Here is a video of the issue with panel resizing. For some reason it doesn't happen for the deployed storybook. Anyone have any idea why?

tmeasday · 2018-08-09T00:54:57Z

Ok, I investigated the panel resizing. I think the reason it doesn't happen on the deployed version is that it only happens in react dev mode. More details here: tomkp/react-split-pane#309

I'm not sure if we should ship this feature until this bug is fixed? It seems like storybook is buggy to have a story hide the addon panel and the layout gets broken.

Can anyone think of an easy workaround?

nm123github · 2018-08-09T23:20:01Z

I could repro this locally. Does seem strange.

tmeasday · 2018-08-10T02:04:29Z

I could also fork / PR react-split-pane to try and get the bug fixed quicker

wuweiweiwu · 2018-08-10T04:09:49Z

@tmeasday If you could PR a fix that'll be amazing. I'm also a collaborator on react-split-pane so i can review it. I just don't have the bandwidth right now to write a fix :(

igor-dv · 2018-08-16T09:38:26Z

Any ideas on how we could fix it properly?

I will try to play with this PR a bit =)

tmeasday · 2018-09-08T04:24:12Z

Ok @igor-dv, that solves it. Let's recommend people use withOptions and deprecate setOptions. Easy.

…-to-options-addon

…etting issue

wuweiweiwu · 2018-09-08T19:12:53Z

Ill open an issue to update react-split-pane to 0.1.83, which would solve the StrictMode issues. Thanks again @tmeasday

Hypnosphi · 2018-09-08T19:44:27Z

I don't think it can be merged to 3.4

wuweiweiwu · 2018-09-08T19:59:04Z

On further investigation, @tmeasday I think I'll have to make additional changes, updating to 0.1.83 breaks the drag action in layouts somehow, it drags extremely slow.

tmeasday · 2018-09-09T11:01:00Z

@igor-dv, yeah this v4 only as params aren’t in v3

@wuweiweiwu any idea why? Afaict the function I changed is only called from componentDidMount and getDerivedStateFromProps, it’s explicitly not called from the drag handler.

Let me know if I can assist!

igor-dv · 2018-09-09T11:10:56Z

We can just add a deprecation warning to v3.4. Just so sad to pollute a beauty of the v4 with deprecations 😢

tmeasday · 2018-09-09T11:20:52Z

Can’t really deprecate something until an alternative is in place!

I think it’s fine to have something (setOptions) that is undocumented and deprecated in our shiny new thing. Previous users will appreciate not having their storybooks break!

wuweiweiwu · 2018-09-10T03:00:00Z

@tmeasday not sure yet. I think getDerivedStaetFromProps being called multiple times per render in dev mode breaks stuff since our size calculation is not pure. I'll dig into it tonight.

edit: seems the new changes also breaks dragging in strict mode. It removed the size invalidation checks

tmeasday requested review from danielduan, Hypnosphi, igor-dv, ndelangen and usulpro as code owners August 6, 2018 05:52

tmeasday changed the title ~~tmeasday/add-params-to-options-addon~~ Add params-based API to options addon Aug 6, 2018

tmeasday added 3 commits August 7, 2018 11:13

Allow per-story setting of options via { options: { … } } & `withOp…

28652d6

…tions`

Removed options stories from Marko

9458fe9

This is just for simplicity. I think it is best if non-framework specific stories all live in official-storybook; that way they are easier to find in the future. cc @nm123github

Update Readme showing how setOptions can take arguments directly.

c865719

cc @nm123github -- FYI -- we made this shortcut so you don't need to call `addDecorator` followed by `addParameters`.

tmeasday force-pushed the tmeasday/add-params-to-options-addon branch from 49694b7 to c865719 Compare August 8, 2018 09:39

Update snapshots

ce3015d

tmeasday requested review from thani-sh, naipath, renaudtertrais, rhalff, rmevans9, saponifi3d, shilman, stijnkoopal, theinterned, thomasbertet and z4o4z as code owners September 8, 2018 04:23

Merge remote-tracking branch 'origin/master' into tmeasday/add-params…

9e9dfe2

…-to-options-addon

tmeasday force-pushed the tmeasday/add-params-to-options-addon branch from c1d93ab to d9ab31c Compare September 8, 2018 04:25

Deprecate setOptions, use withOptions as a decorator to solve res…

d309b91

…etting issue

tmeasday force-pushed the tmeasday/add-params-to-options-addon branch from d9ab31c to d309b91 Compare September 8, 2018 04:26

Hypnosphi added feature request addon: options labels Sep 8, 2018

Hypnosphi approved these changes Sep 8, 2018

View reviewed changes

Hypnosphi merged commit 1dc036e into master Sep 8, 2018

Hypnosphi deleted the tmeasday/add-params-to-options-addon branch September 8, 2018 17:35

wuweiweiwu mentioned this pull request Sep 8, 2018

Update react-split-pane for lib/components to fix StrictMode issues #4147

Closed

wuweiweiwu mentioned this pull request Sep 10, 2018

Updating react-split-pane to version 0.1.84 #4153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add params-based API to options addon #3965

Add params-based API to options addon #3965

tmeasday commented Aug 6, 2018 •

edited

Loading

storybook-safe-bot commented Aug 6, 2018

ndelangen commented Aug 6, 2018

tmeasday commented Aug 7, 2018 •

edited

Loading

nm123github commented Aug 7, 2018

ndelangen commented Aug 7, 2018

tmeasday commented Aug 8, 2018

tmeasday commented Aug 8, 2018

nm123github commented Aug 8, 2018

tmeasday commented Aug 8, 2018

tmeasday commented Aug 8, 2018

igor-dv commented Aug 8, 2018

codecov bot commented Aug 8, 2018 •

edited

Loading

tmeasday commented Aug 8, 2018

tmeasday commented Aug 8, 2018

tmeasday commented Aug 9, 2018

nm123github commented Aug 9, 2018

tmeasday commented Aug 10, 2018

wuweiweiwu commented Aug 10, 2018

igor-dv commented Aug 16, 2018

tmeasday commented Sep 8, 2018

wuweiweiwu commented Sep 8, 2018

Hypnosphi commented Sep 8, 2018

wuweiweiwu commented Sep 8, 2018

tmeasday commented Sep 9, 2018

igor-dv commented Sep 9, 2018

tmeasday commented Sep 9, 2018

wuweiweiwu commented Sep 10, 2018 •

edited

Loading

Add params-based API to options addon #3965

Add params-based API to options addon #3965

Conversation

tmeasday commented Aug 6, 2018 • edited Loading

What I did

STILL OUTSTANDING

How to test

Is this testable with Jest or Chromatic screenshots?

Does this need a new example in the kitchen sink apps?

Does this need an update to the documentation?

storybook-safe-bot commented Aug 6, 2018

ndelangen commented Aug 6, 2018

tmeasday commented Aug 7, 2018 • edited Loading

nm123github commented Aug 7, 2018

ndelangen commented Aug 7, 2018

tmeasday commented Aug 8, 2018

tmeasday commented Aug 8, 2018

nm123github commented Aug 8, 2018

tmeasday commented Aug 8, 2018

tmeasday commented Aug 8, 2018

igor-dv commented Aug 8, 2018

codecov bot commented Aug 8, 2018 • edited Loading

Codecov Report

tmeasday commented Aug 8, 2018

tmeasday commented Aug 8, 2018

tmeasday commented Aug 9, 2018

nm123github commented Aug 9, 2018

tmeasday commented Aug 10, 2018

wuweiweiwu commented Aug 10, 2018

igor-dv commented Aug 16, 2018

tmeasday commented Sep 8, 2018

wuweiweiwu commented Sep 8, 2018

Hypnosphi commented Sep 8, 2018

wuweiweiwu commented Sep 8, 2018

tmeasday commented Sep 9, 2018

igor-dv commented Sep 9, 2018

tmeasday commented Sep 9, 2018

wuweiweiwu commented Sep 10, 2018 • edited Loading

tmeasday commented Aug 6, 2018 •

edited

Loading

tmeasday commented Aug 7, 2018 •

edited

Loading

codecov bot commented Aug 8, 2018 •

edited

Loading

wuweiweiwu commented Sep 10, 2018 •

edited

Loading