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

Make OptionsMenu items controllable in Storybook #304

Closed
wants to merge 3 commits into from

Conversation

micgro42
Copy link
Collaborator

No description provided.

guergana and others added 2 commits November 26, 2020 10:42
- added optional tag to MenuItem
- added tokens for tag
- added styling in OptionsMenu.vue
- updated OptionsMenu story
- updated LookupMenu story
- added a unit test
@guergana
Copy link
Contributor

Looks good. Why is it a draft?

Base automatically changed from menuItem-tag to master November 26, 2020 12:07
@micgro42
Copy link
Collaborator Author

Looks good. Why is it a draft?

When it was created, it was unclear whether people would want it. Setting it ready for review now.

@micgro42 micgro42 marked this pull request as ready for review November 26, 2020 15:55
Copy link
Member

@itamargiv itamargiv left a comment

Choose a reason for hiding this comment

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

Juhu, looks fantastic, I do have one qualm about the menuItems control. Also, I would ask @SaiSan-WMDE where the Options menu should go in terms of the storybook structure she envisioned.

@@ -79,12 +74,11 @@ export function withItems( args: { boldLabels: boolean, selectedItemIndex: numbe

withItems.args = {
boldLabels: true,
menuItems: vegetableItems,
Copy link
Member

Choose a reason for hiding this comment

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

This control feels a bit cluncky, which makes me question how usable it actually is, maybe we should create a couple of controls for only one item from the list? (i.e. label, description, tag)

@sai-san
Copy link
Collaborator

sai-san commented Nov 26, 2020

Also, I would ask @SaiSan-WMDE where the Options menu should go in terms of the storybook structure she envisioned.

Well, first let me say that I'm actually against displaying internal subcomponents (such as the Input or the OptionsMenu) in our Storybook 😑 In my opinion, simply referring/pointing at these from the "finished" component's documentation should be enough.

But I understand the engineers' perspective, so I would say that displaying the OptionsMenu in a separate section (as suggested by this PR) might make more sense than other options such as displaying the component several times in its multiple parents' sections; or creating a separate subsection to collect internal components.

@micgro42
Copy link
Collaborator Author

Making this a draft again as it needs more conceptual work

@micgro42 micgro42 marked this pull request as draft December 14, 2020 10:09
@micgro42 micgro42 closed this Jan 15, 2021
@micgro42 micgro42 deleted the controlableOptionMenuItems branch January 15, 2021 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants