-
Notifications
You must be signed in to change notification settings - Fork 2k
MSD: Change to use a prop to control the last element in PageHeader to control dropdown menu alignment. #107085
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
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether we should push the menu icon to the end when it wraps — it might look odd if there's only one action menu 🙈
To me, if there’s an action menu, it’s better to keep it on the same line as the title. But for other actions, it would make more sense to move them to a new line. Does it make sense 🤔
I think we need designer's input on this!
| alignment="flex-start" | ||
| className="dashboard-section-header__actions" | ||
| className={ clsx( 'dashboard-section-header__actions', { | ||
| 'has-action-dropdown': !! hasActionDropdown, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels weird to introduce this to SectionHeader. It seems the actions should handle it itself
Yeah, for the purchases page, this current exists. I think we should promote the menu items into buttons (the important ones). All pages should have a primary. See context: CHE-286
I see your point, it kind of works, but having two different clickable zones seems a bit weird.
The dropdown menu flying out to the right is designer input :). See DOTCOM-15116 |
Yes. Generally, all of them should become icons in the header if there isn’t enough space. However, I think it would be better to promote it to a text button, like
Oh I see 👍 |
arthur791004
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we only have 2 use cases now. How about just wrapping primary actions with HStack? Or could we create something like PageHeader.ActionMenu component and replace the DropdownMenu with it? WDYT? 🤔
Right. These three-dots menus should strictly be used for overflow. Our standard should be max 2 buttons, everything else goes into the three-dot menu. We should never just have a three-dot menu.
I thought about that but didn't think we used that pattern until I noticed today we do that for the header bar. I think that's a good idea. |
|
Thanks @StevenDufresne. I think your proposed solution makes more sense and I also think @arthur791004 has brought a good idea. Let's move forward with a mix of yours. I'll close my PR. 😄 👍🏻 |
|
Opened the alternative @arthur791004 suggested in #107117. |
Alternative: #107072
Related to: #107003
Proposed Changes
This PR attempts to improve devEx for PageHeader consumers, who have drop-downs in their
action. Our preferred small screen layout is to right-align the menu.The problem right now, is that the consumer is responsible for handling that layout. See this comment for context.
Here I've added a prop,
hasActionDropdown, that tells the section header that it has a dropdown in the actions list, and it adds a custom style ,has-action-dropdownwhich sets up CSS to right align the last item when the actions wrap.I'm not sure I'm in love with the prop name, but I can't think of one that's short and descriptive.
Screenshots
Testing Instructions
Visit the following pages on mobile/desktop:
/v2/sites/:site/v2/domains/:domain/dns/v2/me/billing/purchases/:purchaseIdPre-merge Checklist