Skip to content
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

Addon-controls: Copy as URL and load from URL #12291

Closed
shilman opened this issue Aug 28, 2020 · 25 comments
Closed

Addon-controls: Copy as URL and load from URL #12291

shilman opened this issue Aug 28, 2020 · 25 comments

Comments

@shilman
Copy link
Member

shilman commented Aug 28, 2020

Same as #11604, but for args/controls.

This is currently possible by the knobs addon:

Addons___Knobs___withKnobs_-_Select_Knob_⋅_Storybook

We should also make sure that the eject button works per #12675 at the same time

@togakangaroo
Copy link

To be clear why this is an issue, its not simply about sharing links. During development I want the state of the args to persist between re-renders. For example right now I'm developing the top bar for an app with the current user display. It a photo url that appears in the upper right or an account-circle icon if one is not provided.

I don't want to hard-code a photo url in the codebase because it right now just points to my google profile photo over the internet, but its also a real pain in the ass to have to constantly paste the url in every time it re-renders

@tmeasday
Copy link
Member

tmeasday commented Oct 8, 2020

@togakangaroo I would strongly encourage you to encode it as a story, if it is useful now, it'll be even more useful later when you have to come back to this component and fix a bug and you can't quite remember how it all works.

In any case, it sounds like HMR isn't working for your SB. Or is this a bug in args and HMR?

@gullerya
Copy link

I've mentioned that in the #12728 PR, but believe that it should also be stated here (due to some optional workaround suggested above) - the possibility to create a 'deep-link' into the components state with all of the controls set as per user - is quite needed functionality.

The proposal of 'flattening' the deep state into a separate stories is inconvenient when component has few dozens of possible state permutations, IMHO.

@jfairley
Copy link

To add another use case:

My team maintains a component library where the only "frontend" is storybook. We use cypress to test our components, and in each test, we target the ejected iframe directly. (In fact, we have to target the iframe, because cypress can't interact with elements within an iframe.) We initialize the component state via query parameters supported by addon-knobs.

I know that addon-knobs is deprecated in favor of addon-controls, but we can't currently upgrade to addons-controls without query params or fundamental changes to how we test components. I seriously fear that without added query param support, we may never upgrade to v7 (where it sounds like addons-knobs will be dead).

@juristr
Copy link
Contributor

juristr commented Jan 19, 2021

My team maintains a component library where the only "frontend" is storybook. We use cypress to test our components, and in each test, we target the ejected iframe directly. (In fact, we have to target the iframe, because cypress can't interact with elements within an iframe.) We initialize the component state via query parameters supported by addon-knobs.

That's the exact same case we preconfigure Storybook for our Nx users. We even automatically give people the option to setup Cypress based e2e tests when we configure Storybook for them. Now not having this deep-linking possibility kinda makes that feature unusable...

@tmeasday
Copy link
Member

@jfairley / @juristr - not to say we don't want to fix this (we do!) but just to understand the use case here, I'm trying to understand why the suggested workaround (of defining a story for each args/knobs combination) wouldn't work for you? Is it just a convenience thing?

@jfairley
Copy link

jfairley commented Jan 20, 2021

@tmeasday yes, it is part a convenience thing and part a preference thing.

First, convenience: The amount of flexibility that comes from injecting variables is hard to replicate, and more importantly maintain, by having many almost-the-same stories built only with slight variations. Most simply this goes against the DRY principle, and reasons why that principle exists fits here.

Second, preference: This one may be more important for me actually. If I went ahead and created a bunch of stories to support my test cases, I don't really like what my storybook would end up being; I feel my storybook sidebar would become cluttered and messy.

Roughly, I consider my test suite as just another user who might use storybook. Devs creating test cases are merely calling out specific scenarios that I'd expect any real user to try out via the UI controls. That, actually is the beauty of knobs or controls, that the user has full control over component inputs without having to edit the stories themselves. I want my dev and qa teams to continue to have the same flexibility.

@tmeasday
Copy link
Member

Thanks @jfairley, can I dig a little deeper?

Most simply this goes against the DRY principle, and reasons why that principle exists fits here.

Just to be clear though, in your example you would create a test case in cypress that goes something like:

  1. Go to $SB_URL?knob1=X&knob2=Y.....
  2. Do stuff...

And the alternative is:

  1. Create a story in SB w/ {arg1: X, arg2: Y}, w/ id=xyz
  2. Create a test in cypress that goes to $SB_URL&id=xyz
  3. Do stuff...

So in a sense we have just moved the knob1=X&knob2=Y... code into the story, right? I'm not seeing how we are repeating ourselves any more than before? Do I have this wrong?

I feel my storybook sidebar would become cluttered and messy.

Fair enough. Although this kind of thing feels a little bit like a user preference, like maybe it would be better to somehow tag stories as "testing" stories and have a way to hide such stories from the sidebar? I wonder if other users (e.g. your QA team) would actually prefer those stories to be visible in the sidebar to make debugging their cypress tests easier?

Devs creating test cases are merely calling out specific scenarios that I'd expect any real user to try out via the UI controls.

What's a "real user" in this case? Do you mean like a component library consumer?

I want my dev and qa teams to continue to have the same flexibility.

We agree that control over arg values via URL is super useful of course! Is the role an issue here? Like do the QA team not have the ability to easily write stories?


As I ask these questions I'm wondering if there's a bit of a tension here between "stories you want users to see" and "stories that are useful for development/QA" (i.e. edge cases). I wonder if SB needs a semantic distinction between the two somehow when creating docs.

@shilman
Copy link
Member Author

shilman commented Jan 21, 2021

@jfairley @tmeasday There's also this: #9209

@jfairley
Copy link

@shilman That feature request about hiding stories is interesting. I subscribed to see how that one goes.


@tmeasday Your clarification of knob args (like I do today) vs a story with baked-in args is clear. We're on the same page with respect to the alternatives. 👍🏼

What's a "real user" in this case?

In my specific case, I am arguing from more of a hypothetical than others might. We don't currently publish our components or stories publicly (though we may in the future), so my users don't extend beyond the engineering team (developers, testers, product owners). But I think a lot of my opinion on this comes from the philosophy of not letting tests drive source code. It's a loose comparison, but in this case I'm trying to treat my stories more as documentation that are defined by the product use cases rather than by tests, which at times might be pressing on uncommon edge cases.

Now all that said, my stories certainly aren't perfect. I have plenty of examples where I created new stories to fit test cases (as you're suggesting). I'm just cautious of that being what I have to do.

Like do the QA team not have the ability to easily write stories?

Here's where things get a little less pleasant. First, yes, the QA team could write stories, but they don't yet. When devs write cypress test cases, they're bundled in the repo and run in CI along site Jest unit tests. Being in the same repo, devs can very easily add or edit stories. The QA team operates on their own though. Their tests live in separate repos and run against a hosted static storybook instance (which doubles as the product owner documentation / demo / playground). So, while the QA team could commit new stories into the main repo, they don't currently, so we would have to change our process a bit.

We agree that control over arg values via URL is super useful of course!

Yes, it's more about "we don't do it that way" than "we can't do it that way."

I do think hidden stories could be an ok middle ground, particularly if exposing controls is difficult on your end. QA adding stories would still be less than ideal, but not the worst thing.

@gullerya
Copy link

gullerya commented Jan 21, 2021

So in a sense we have just moved the knob1=X&knob2=Y... code into the story, right? I'm not seeing how we are repeating ourselves any more than before? Do I have this wrong?

Actually, your suggested patter IS having lots of repetition, IMHO.
Let's take an easy example of yours:

  • 2 args (arg1 can have 2 values; arg2 can have 3 values)
  • your proposal is to write 6 stories, where arg1's each value will be repeated 3 times and arg2' each value will be repeated 2 times)
  • so it's (very) roughly about n * m (n args, m distinct values in each in average) stories, where each value will be repeated about (n - 1) * m times

You could say - 'well, you'll need all those permutations anyway, at least somewhere in tests, if to speak that use-case', and you are right. But it's an absolutely different story to make up permutations by URL, dynamically on consumer side, and to actually bake in all those stories.

@juristr
Copy link
Contributor

juristr commented Jan 21, 2021

why the suggested workaround (of defining a story for each args/knobs combination) wouldn't work for you? Is it just a convenience thing?

@tmeasday Yes and no. We could obviously create different story setups for all the various combinations and then even hide those we don't like in our storybook (as discussed above). Tbh that sounds a bit of a workaround for me.

It makes sense to create different story configs that I also want to show on my storybook page as they might highlight the most prominent configurations of a given component.
But for all other scenarios, when I have a component with different inputs, those are variables for me that the user should be able to interact live on my storybook page to see how the component behaves.

In addition, in Nx we've automated that for the user, to also allow to generate Cypress tests that control those inputs dynamically and test for the behavior of the component. Basically automating the user interaction with the component. And for exactly that interaction we need the possibility to obviously control these component inputs via the URL when we access the iframe that renders the story.

To give you a better understanding, here are two vids that demo that:

Obviously, that brings a lot of benefits to the user and motivates them to use the Storybook and Cypress integration together, as they can design and develop the components in isolation with Storybook's capabilities and in addition even test them in isolation (as a user would) with Cypress :)

@ghengeveld ghengeveld self-assigned this Jan 25, 2021
@ghengeveld
Copy link
Member

ghengeveld commented Jan 25, 2021

It seems like there's an unaddressed issue here. Some of you mentioned you don't like to have separate stories for each of the controls' commonly used settings, because it clutters up the sidebar with a ton of mostly identical stories. Controls is part of a subject we refer to as "combinatorial testing", where one component has many combinations of props (permutations) that need to be tested. We're hashing out various ways to tackle that use case.

One of the sub-problems is rendering the various combinations in a sensible way. Having them as separate stories is inconvenient, not only because it clutters the sidebar, but also because it makes snapshot testing more expensive (as typically 1 story = 1 screenshot) and it's often more convenient to view variants side-by-side. That's why I've recently been experimenting with a story wrapper that renders a grid of component instances in various states, as a single story. Something like this example. The trick is to make this work equally for all frameworks. Would this sufficiently alleviate the issue of cluttering the sidebar (besides the option to hide stories)?

I'd like to point out that describing all relevant component variants is exactly what Component Story Format is for. Encoding variants in a URL in some unrelated test file instead of in the CSF file undermines the purpose and intent of CSF. You may not currently experience the benefits of doing it that way. That will hopefully change with new addons and tools that build on top of CSF.

Being able to share the state of Controls via the URL is a useful feature for the sake of collaboration, so we still want to fix this.

@tmeasday
Copy link
Member

@gullerya -- I'm not sure I am quite saying what you think I am.

your proposal is to write 6 stories, where arg1's each value will be repeated 3 times and arg2' each value will be repeated 2 times)

My proposal is just to write stories for whichever combinations of args are referenced (via URL as proposed in this feature) in other places. I'm not suggesting we get rid of the controls feature entirely, which is what enables this combinatorial explosion in the number of arg combinations.

If someone is making reference to each of the 6 combinations in 6 different cypress tests (or otherwise), then I guess I am proposing writing 6 stories. But, I suspect if each combination is worth writing a test for, it is probably worth having a story for too.

@jfairley

I do think hidden stories could be an ok middle ground, particularly if exposing controls is difficult on your end.

It's not that it's difficult (and it is something we are going to do!), it's just that some of these use cases feel like anti-patterns to us (we want you to write stories!), so I just want to understand what's stopping people from writing stories and see if we can remove that impediment, rather than just adding a feature that makes it easier to not write them ;).

QA adding stories would still be less than ideal, but not the worst thing.

Why is that? Can you elaborate?

@juristr

And for exactly that interaction we need the possibility to obviously control these component inputs via the URL when we access the iframe that renders the story.

Watching that video I'm not really sure I see how it is required to change the arg (/knob) values via the URL parameters, as opposed to editing a story. I would strongly advocate that your generator did the following when creating a cypress test:

  1. Generate a story with a default set of arg values:
export CypressTestOne = Template.bind({});
CypressTestOne.args = {
  label: 'foo',
  // ...
}
  1. Create a cypress test that simply browses to http://.../iframe.html?id=cypress-test-one
  2. When the user wants to tweak the test they simply open the CSF file and alter the story args.

Apart from the other benefits of having it "storified", I would suggest it is going to be a lot easier and less error prone to alter a line in a CSF file than trying to find and modify a URL query parameter buried in a URL in a test case, especially when you consider things like URL serialization etc.

PS it is really cool how your generate generates the CSF file automatically from the components! We want to add a feature like that to Storybook itself via the CLI. If your team is interested in contributing it back to Storybook that would be awesome!

@jfairley
Copy link

jfairley commented Jan 26, 2021

QA adding stories would still be less than ideal, but not the worst thing.

Why is that? Can you elaborate?

Put most simply, in the scenario that QA e2e tests exist in a separate repo, it represents them collaborating in two repos instead of one. The lifecycle is much more cumbersome that way. Recall, that QA is coding against a built-and-hosted version of storybook.

When simply writing tests against a runtime-parameterized story, the dev cycle is quick:

  1. Write a test.
  2. Run the test.
  3. See and fix test issues.
  4. Re-run the test.
  5. Rinse-and-repeat.

When adding stories before writing tests, everything above happens, plus:

  1. Add a story.
  2. Wait for CI to build.
  3. Deploy (assuming you are like most and don't have fully automated deployment).
  4. < write your test >

But, I suspect if each combination is worth writing a test for, it is probably worth having a story for too.

IMO, this is usually true for happy-path demonstrations. I see stories as "this is how you're supposed to use my component."

But good testing often focuses just as much on the sad-path scenario. This is JavaScript after all.

  1. I know you say this prop accepts an ISO date, but let's see what happens if ...
  2. Oh, you accept one of 4 enumerated inputs, but let's see what happens if ...
  3. etc.

These sad-path scenarios aren't cases that I want enshrined as stories. It's basically saying: "Look, here are 10 examples of how when you disregard the documentation, bad things happen." There's no value in that outside of testing. Not to mention, show the wrong way to use something, and invariably, you're going to find a user/developer who isn't paying enough attention to see that it's a sad-path case, and takes it as instruction, coding it like that in the real app.

@jfairley
Copy link

@ghengeveld that's an interesting take, putting multiple variants of a component on the same page. It's a creative way to cut down on sidebar clutter, but I would make cypress (non-snapshot) testing difficult only in that the css selectors need to be made more specific, though that's not a huge deal. I should point out that buttons are one of the smaller components one might want to show off, so as component sizes grow (think a panel or list component), this tactic becomes harder to maintain. But I like the idea!

@sefaltay
Copy link

sefaltay commented Feb 2, 2021

I believe my question is related to this issue.
Our team is using selenium yet we want to test directly from iframe and want to change controls through URL. Is it possible or not? I believe ongoing discussion is somewhat related :)
Ex:
.../iframe.html?id=basic-button&buttontext=sampletext&disabled=true

@juristr
Copy link
Contributor

juristr commented Feb 6, 2021

@sefaltay Looks like this is what @ghengeveld is working on currently

@shilman
Copy link
Member Author

shilman commented Feb 17, 2021

Ta-da!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.2.0-alpha.29 containing PR #13803 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Feb 17, 2021
@juristr
Copy link
Contributor

juristr commented Feb 18, 2021

@shilman Awesome! gonna try 🙇

@sefaltay
Copy link

now are we able to see changes in "control" in sync with URL ?

@shilman
Copy link
Member Author

shilman commented Feb 20, 2021

@sefaltay yes, as you update the controls the URL updates in realtime

@jfairley
Copy link

Noice! I can't wait to give it a try!

@khats
Copy link

khats commented Feb 21, 2021

@shilman awesome works with primitives, but controls with type object is not handled :(

@shilman
Copy link
Member Author

shilman commented Feb 21, 2021

@khats correct. we have a few possible approaches to deal with that and will do our best to address it in a future iteration cc @ghengeveld @amartinez62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants