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

refactor(components, protocol-designer): menuList and MenuItem cleanup for helix #17257

Open
wants to merge 5 commits into
base: edge
Choose a base branch
from

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Jan 13, 2025

closes AUTH-1153 AUTH-1152

Overview

MenuList and MenuItem clean up & refinement as part of helix work

Test Plan and Hands on Testing

check adding liquids overflow menu in PD, step overflow menu in protocol steps in PD, check overflow menus in the app - i did some smoke testing and i think they work as expected, but please do some additional smoke testing to be safe

Changelog

clean up the storybooks for both
create const to be used for storybook
use MenuList in a few places in PD

Risk assessment

low

@jerader jerader requested review from a team as code owners January 13, 2025 20:02
@jerader jerader requested review from mjhuff, koji and ncdiehl11 and removed request for a team January 13, 2025 20:02
@@ -0,0 +1,80 @@
// constant used for filtering out controls in Storybook that include StyleProps
export const allDefaultStorybookControlsWithStyleProps: string[] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, can you elaborate on why we need this? Also, can we pull from somewhere like style-props.ts?

Copy link
Contributor

Choose a reason for hiding this comment

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

As Nick said, you can pull all items without defining a new const.

the easiest way would be exporting STYLE_PROPS in style-props.ts and feed it to a stories.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mel has a storybook requirement where if you extend your component's props to include StyleProps, each of those props show up in storybook as something you can control. This constant removes that list. However, i am not sure if this is the best approach to remove that list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops didnt' see Koji's response initially when i wrote my comment -- sounds good, I'll clean it up and export STYLE_PROPS

components/src/atoms/MenuList/index.tsx Show resolved Hide resolved
@jerader jerader requested a review from koji January 15, 2025 18:07
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.

3 participants