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

Add support for grouping conditions with and/or operators #76

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

DiogoDoreto
Copy link

@DiogoDoreto DiogoDoreto commented Feb 15, 2024

In our project we would benefit from extra conditions for our Story's controls, to better match our more complex polymorphic component parameters.

Please check the new tests for example usage.

@kasperpeulen
Copy link
Contributor

cc @tmeasday @shilman

@shilman
Copy link
Contributor

shilman commented Feb 15, 2024

Cc @kylegach

@kylegach
Copy link

Nice work, @DiogoDoreto!

The API is as straightforward as I can imagine it being. I recommend that we only document one level of nesting, though, even if it's implemented recursively as it is here.

That said...

to better match our more complex polymorphic component parameters

I wonder if there's something better we could offer for this use case, that wouldn't require manually authoring a bunch of complex conditions. Whatever that is, it could likely be implemented as an auto-generation of this feature. So this PR is still valuable.

@DiogoDoreto
Copy link
Author

Thanks, @kylegach!

Regarding your last point, what we really thought about initially was to have the options of a control to be dynamic and based on the values of other controls. Since this was a major change from the current requirement of being statically defined we tried to avoid it. The current solution is to multiply the number of controls with pre-calculated options and predefined rules for visibility — where multiple conditions come in handy. And yes, we are autogenerating those :)

@darkgrayknight
Copy link

This would be great. Is anyone looking at this PR and going to see about merging it in?

@m4olivei
Copy link

Echoing that this would be fantastic. In our case, we're migrating from knobs to controls and finding control conditionals to be way too restrictive. While I like the idea here, I wonder if there would be yet more complex conditions that would require an escape hatch from the DSL here. This was brought up in storybookjs/storybook#19135, where the suggestion was to allow developers to provide a function that would receive args and return true or false.

Is there a reason that couldn't also be added here, or done instead of this approach? I suspect it has to do with what @DiogoDoreto said:

Since this was a major change from the current requirement of being statically defined we tried to avoid it

Although I don't quite understand the requirement here or where it comes from?

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

Successfully merging this pull request may close these issues.

6 participants