-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
No scrollbar in Select component when menu has dividers #6804
Conversation
Generate changelog in
|
no select scrollbar with dividersBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
alpha orderBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
I think going over the |
There's no changes to the menu in this PR, except when inside a select popover I checked for good measure and in the generated docs preview you can see the styling menu is unaffected: Note in #6786 since the -5px margin is removed there is a slight visual regression in the screenshot where the border no longer goes to the edge of the menu, which seems to be what the intent of that style is. |
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.
Menu Items Flush
Is it intended that the menu items themselves are flush with the border of the popover after these changes? I see from your description that we wanted to make the horizontal dividers flush, which I agree with.
Here's the before picture again for reference. I think the intended visual design was to have a bit of spacing around them menu item since the light blue hover background has a slight border radius. That border radius doesn't look as great if the background is how flush with the border.
Suggestion
I'm wondering if we should tweak @mixin menu-divider()
's implementation to instead be a ::before
pseudo-element that is "out of flow".
The negative margin would then only be on the pseudo-element driving the horizontal border. Since the pseudo-element is "out of flow", we would eliminate unintended CSS layout calculations (like this one) entirely in the future.
a6eec59
to
94a5ca4
Compare
Thanks for the close look and good catch @gluxon! I wrapped up this approach since it was easy to add back the margin one more time for the items, but given how much back and forth this is I may try the psuedo element approach quickly as well. As a nice to have - it looks like there was a small stying weirdness where there is empty space padding to the left but not right of the selection. With fixing the scrollbar overflow this also sizes the menu items correctly to have padding on both sides. develop![]() ![]() this PR![]() ![]() with scrollbars showing only when scrolling still looks good |
add side padding around selection highlightBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
} | ||
} | ||
|
||
.#{$ns}-menu-item { |
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 wonder if there may be other classes we need to target with adding back the margin we take away above...
From what I could tell from the docs it seems like only elements that get this bp-menu-item
class are intended to go into a menu, but I suppose it may be allowed by the API to develop custom menu items that may now shift around with this change?
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.
ahh, I think this was a good note... pushing to make the divider and menu items not be a special case got me to bring back the padding on the actual menu, which allows the divider to keep it's normal negative margin
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.
Nice!
better approach?Build artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
// offset the padding added by the select componenent so menu can remain full width with padding on either side | ||
// for menu items |
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.
nit: Is it worth a comment that this is specifically to allow dividers to be flush with the popover's edge?
Very nice! 😁 |
Co-authored-by: Brandon Cheng <gluxon@users.noreply.github.com>
Update packages/select/src/components/select/_select.scssBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fixes #6780
Checklist
I don't think these are relevant for this PR
Changes proposed in this pull request:
For a menu divider inside of a select-popover, we already have applied a 5px padding inside the popover. This means to get to full popover width we need to apply the -5px margin to the outer list element, and therefor also undo the -5px margin on the menu divider.
This PR is a variation on #6786, which correctly identified that the -5px margin on the menu divider is responsible for the scrollbar being added, but this PR additionally applies this -5px margin on the list element so that the divider continues to take up the full width.
Reviewers should focus on:
Screenshot
Before
![Screenshot 2024-05-15 at 2 20 22 PM](https://private-user-images.githubusercontent.com/14102129/330917313-e195627f-5755-4fd8-9b6a-4cc153ec545d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzgyNzQwNDQsIm5iZiI6MTczODI3Mzc0NCwicGF0aCI6Ii8xNDEwMjEyOS8zMzA5MTczMTMtZTE5NTYyN2YtNTc1NS00ZmQ4LTliNmEtNGNjMTUzZWM1NDVkLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAxMzAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMTMwVDIxNDkwNFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWNjZDA2OWVhNWM0YTk0YmY4YzkxMTM4MWQ0OGU2YzJjYTgwMWNkYzAzMTg1M2ZjNTY5ZDdkZjhmMGY1NGE4YmQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.X1RMUpj4yCj1f_Js8IfsFgZY74d2NN8SrvVNdZlsbpA)
After
![Screenshot 2024-05-15 at 2 19 48 PM](https://private-user-images.githubusercontent.com/14102129/330917339-b5e54a35-d9ae-474e-bd5c-e4664cb2c08c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzgyNzQwNDQsIm5iZiI6MTczODI3Mzc0NCwicGF0aCI6Ii8xNDEwMjEyOS8zMzA5MTczMzktYjVlNTRhMzUtZDlhZS00NzRlLWJkNWMtZTQ2NjRjYjJjMDhjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAxMzAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMTMwVDIxNDkwNFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTgyZDgxZmI1ODlmNWYyNDJlNDIwNGJiOTJhM2E0MDEwNTAxMDU0MTM2N2Q0YzE0ZWU1YzEwMjJhNGFjOGMzMzQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.TRk063hE7_WA1ogruGahn4Ug6eq6rPqif3ns1vP1BBg)
*note right and bottom padding outside of list are now also removed. this seems to match what we do in the menu popover