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

1827 multiple menu buttons #1849

Merged
merged 3 commits into from
Sep 8, 2022
Merged

1827 multiple menu buttons #1849

merged 3 commits into from
Sep 8, 2022

Conversation

LuLaValva
Copy link
Member

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

  • Dropdown menu spanned whole set of buttons when multiple were placed next to each other.

Notes

  • I modified dropdown-base instead of just menu-button for expandability, it did not break anything as far as my testing is concerned, so I think it is preferable.

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

@LuLaValva LuLaValva self-assigned this Aug 26, 2022
@LuLaValva LuLaValva changed the base branch from master to 15.0.0 August 26, 2022 19:05
@agliga
Copy link
Contributor

agliga commented Sep 1, 2022

Looks good overall, but two comments on this that need to be fixed

  • on ltr, we need to do left: unset and right: 0
  • on --reverse we need to also unset the left and set right to 0. For ltr we should do the opposite.

This is whats happening on reverse menu button.
Screen Shot 2022-09-01 at 12 24 25 PM
But setting right to 0 and left to unset works:
Screen Shot 2022-09-01 at 12 24 58 PM

For LTR, it looks fine until you have a large item:
Screen Shot 2022-09-01 at 12 25 35 PM
But setting right to 0 it works fine
Screen Shot 2022-09-01 at 12 25 42 PM

I think with those changes we should be good.

Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Approving with the condition that Andrew's recommended fix be applied for certain use cases.

@LuLaValva LuLaValva requested a review from ArtBlue September 6, 2022 20:29
}
}

.dropdown-reverse(@component) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...is this arg doing anything? Doesn't look necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this argument isn't needed.
The rest of the PR looks good though.

@LuLaValva LuLaValva merged commit 4b3705b into 15.0.0 Sep 8, 2022
@LuLaValva LuLaValva deleted the 1827-multiple-menu-buttons branch September 8, 2022 22:25
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.

menu: menus for multiple form themed menu buttons aren't displayed properly
3 participants