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

ArgControl: Add Union Type Arg Control #22912

Closed
wants to merge 15 commits into from
Closed

Conversation

chakAs3
Copy link
Contributor

@chakAs3 chakAs3 commented Jun 5, 2023

Closes #

What I did

This PR introduces additional controls to allow for switching between types (ex: string | number ), as existing Storybook functionality only provides one control for this purpose.

unionArgControl.mov

How to test

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@chakAs3 chakAs3 requested a review from a team as a code owner June 5, 2023 08:09
@socket-security
Copy link

socket-security bot commented Jun 5, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented Jun 5, 2023

@cdedreuille Hey Charles. Can you take a look at this to get an idea of how, design-wise additional controls for union types should look like?

@chakAs3 chakAs3 mentioned this pull request Jun 8, 2023
5 tasks
@shilman shilman self-assigned this Jun 10, 2023
@chakAs3 chakAs3 changed the title Feat:(ArgControl) add union control for union types ArgControl: Add Union control for union types Jun 22, 2023
@chakAs3 chakAs3 changed the title ArgControl: Add Union control for union types ArgControl: Add Union Control for union types Jun 22, 2023
@chakAs3 chakAs3 changed the title ArgControl: Add Union Control for union types ArgControl: Add Union Type Arg Control Jun 22, 2023
@chakAs3 chakAs3 marked this pull request as ready for review June 23, 2023 16:43
@yannbf
Copy link
Member

yannbf commented Aug 22, 2023

@cdedreuille @domyen could you please chime in? Thank you!

Comment on lines 36 to 40
'&[type="number"]': {
/* without this we will have inconsistant height */
padding: '0px 10px',
},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's an argument here to set a fixed height on the input instead of modifying paddings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i had these options, i went with padding to avoid fix height, i guess only design team can make the call 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me know which one we go with

Copy link
Contributor

@cdedreuille cdedreuille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the video you shared @chakAs3, I can see that when you are activating the boolean, the height get increased. Can you make these changes:

  • Make sure all inputs are vertically aligned
  • Make sure the height is never increased (with exception for Textarea that could be increased of course)

@cdedreuille
Copy link
Contributor

After thinking more about it, I'm a bit confused by how it should work. From what I see we have 2 parts for it:

  • The selected input that will serve as a control
  • Option to change the input type - For that one I think we need to explore a bit more how to do it. Listing the options as buttons next to it doesn't seem to be the optimal options.

As a quick guidelines:

  • I would keep the selected input always on the left to be aligned with the other controls.
  • I would perhaps explore a Select button "Change type" instead of listing all options directly.

@chakAs3
Copy link
Contributor Author

chakAs3 commented Aug 31, 2023

  • I would perhaps explore a Select button "Change type" instead of listing all options directly.

i'm with this partially, it is not always good to add an extra click when you can have one, most of time user will have 2 to 3 type, i suggest to have 3 max to display more than that 'Show all types'

@chakAs3 chakAs3 requested a review from JReinhold as a code owner September 10, 2023 20:37
@cdedreuille
Copy link
Contributor

cdedreuille commented Sep 11, 2023

most of time user will have 2 to 3 type

@chakAs3 We can't rely on assumption or personal biases to make a call on these ones. I would much prefer we take the time to create a flow that would work for more use cases. Also, I don't think this would be a good behaviour to mix too many things on each line. I'm happy to take a bit of time in the next few days to find a solution to this with you. Ping me on Discord if you want.

@chakAs3
Copy link
Contributor Author

chakAs3 commented Sep 12, 2023

I don't think this would be a good behavior to mix too many things on each line.

I believe it's generally not advisable to mix too many elements on a single line unless there's a compelling reason to do so. It's essential to consider the tradeoffs involved.

We can't rely on assumptions or personal biases to make a call on these ones.

I completely agree. That's why I suggest we consider conducting a canary release on this PR, allowing our users to provide valuable feedback. In the meantime, we can take the time to come up with the best solution, one that's supported by user feedback.

In my opinion, Storybook is a tool that helps developers create and document components conveniently. It should also help in identifying bad practices and behavior in code.

For instance, if a user decides to use a component with a union of 10 types, which is generally considered a less than ideal coding practice, Storybook can serve as a helpful tool. It can visually demonstrate to the user that mixing all these types may not be a good approach. This, in turn, can prompt the user to rethink their approach and find a better solution.

In fact technically user can have max four: boolean, number, string, and object for union types.

In terms of UX, providing users with the option to test their union type props using multiple controls (even 3+) is preferable to forcing them to use a single control. Storybook's goal should be to empower users, not impose restrictions on them

@cdedreuille
Copy link
Contributor

I think the best way to move forward here would be to explore all the different options in design first before coming back to code just to make sure we take the right decision. We can use the design to share this to the community and ask feedback. Creating a canary will only show one version and it is quite demanding for such a small update. I would advise to stick to design for now and use Discord to share some sketches.

I would be happy to support you to mock these ideas out if you want.

@cdedreuille
Copy link
Contributor

I think a lot of people could benefit from this feature but to make it right, it would be great if you could write an RFC detailing all the specifics of this features, how the API will be structured and what are the different design options we could go. I would be happy to share some design thoughts on this RFC if needed.

For now I'll close this PR. If we find a good solution after writing the RFC, we could reopen it.

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

Successfully merging this pull request may close these issues.

5 participants