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

[ActionMenu] - fixes after RefreshUI tests #775

Merged
merged 5 commits into from
Sep 13, 2023

Conversation

marcinsawicki
Copy link
Contributor

@marcinsawicki marcinsawicki commented Sep 12, 2023

Description

Changed the trigger implementation, it uses the default button element, which wraps the element, with the possibility to modify it by triggerClassName prop.
Also added groupHeader prop to options to better group menu elements.

Storybook

https://feature/action-menu-trigger-fix--613a8e945a5665003a05113b.chromatic.com/?path=/story/components-actionmenu--keep-open-on-item-click

Checklist

Obligatory:

  • Self review (use this as your final check for proposed changes before requesting the review)
  • Add reviewers (livechat/design-system)
  • Add correct label
  • Assign pull request with the correct issue

@marcinsawicki marcinsawicki added bug Something isn't working feature New feature or request labels Sep 12, 2023
@marcinsawicki marcinsawicki added this to the Refresh UI milestone Sep 12, 2023
@marcinsawicki marcinsawicki self-assigned this Sep 12, 2023
</button>
</li>
))}
{options.map((o, i) => getOptionElement(o, i))}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{options.map((o, i) => getOptionElement(o, i))}
{options.map(getOptionElement)}

const handleItemClick = (itemOnClick: () => void) => {
itemOnClick();
const handleItemClick = (itemOnClick?: () => void) => {
itemOnClick && itemOnClick();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
itemOnClick && itemOnClick();
itemOnClick?.();


if (!keepOpenOnClick) {
setIsVisible(false);
}
};

const getOptionElement = (option: IActionMenuOption, index: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to put this element as a separate component into a components folder

@marcinsawicki marcinsawicki merged commit 122e95b into main Sep 13, 2023
@marcinsawicki marcinsawicki deleted the feature/action-menu-trigger-fix branch September 13, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants