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

Move Select's toggle button padding to its children elements #1777

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Nov 13, 2024

This PR changes how padding is handled in the Select's toggle button, so that instead of having a surrounding padding in the button itself, we replace it with the corresponding padding to its two children.

This is important because the first children truncates its content via tailwind's truncate. This class, among other things, adds overflow: hidden, but we want to allow content to overflow as long as it doesn't grow outside of the button limits.

Before:

select-button-padding-before-2024-11-13_15.06.53.mp4

After:

select-button-padding-after-2024-11-13_15.06.10.mp4

Visually, there should be no changes, but the element with the truncate class now spans to the button limits, making its behavior more intuitive.

Note

The motivation for this change is described in this thread hypothesis/lms#6820 (comment)

@acelaya acelaya requested a review from robertknight November 13, 2024 14:09
@acelaya acelaya changed the title Move button padding to its children elements Move Select's toggle button padding to its children elements Nov 13, 2024
@robertknight
Copy link
Member

This is important because the first children truncates its content via tailwind's truncate. This class, among other things, adds overflow: hidden, but we want to allow content to overflow as long as it doesn't grow outside of the button limits.

I'm not clear on what you mean. The first child still has truncate applied to it, which means that overflow is still hidden. Do you mean that content which previously overflowed into the button's padding region is now instead part of the content's padding region, and thus not overflowing?

Can you clarify how this helps in the context of hypothesis/lms#6820?

@acelaya
Copy link
Contributor Author

acelaya commented Nov 13, 2024

This is important because the first children truncates its content via tailwind's truncate. This class, among other things, adds overflow: hidden, but we want to allow content to overflow as long as it doesn't grow outside of the button limits.

I'm not clear on what you mean. The first child still has truncate applied to it, which means that overflow is still hidden. Do you mean that content which previously overflowed into the button's padding region is now instead part of the content's padding region, and thus not overflowing?

Can you clarify how this helps in the context of hypothesis/lms#6820?

So, without applying any extra styles to the Select's button, this is how it looks when we add the count badge

image

It grows a bit more than when it only has "plain text", which a) makes it a little bit clunkier, and b) can make selects have inconsistent heights depending on the content of the button, as the badge is displayed only sometimes.

Additionally, if some selects display the badge and some not, some styles break (which we could address separately, but we still have point a).

image

So, in order to compensate for the extra height added by the badge and ensure Selects look closer to the designs, I tried to apply negative margins to the badge, but then this happens:

image

The badge gets cut, because the overflow is hidden at this element:

image

So, what this PR does is moving the padding to the same element with truncate, so that we still apply overflow: hidden and text-overflow: ellipsis, which is correct, but we allow overflowing vertically over the padded area, as long as it doesn't grow outside the button borders.

That's what I would intuitively expect.

image

image

Of course, we could argue if this is in fact the right problem to solve. Maybe we should not apply truncate at all, or allow it to be conditionally applied, but that has other side effects.

Or maybe the negative vertical margin on the badge is the hack that's driving everything in the wrong direction, but I also don't see a way to avoid that without altering the button's layout.

I have tried a couple different combinations, but with all the scenarios we support with Select, this was the less disruptive and less prone to introduce a regression somewhere else.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

OK, thanks for the explanation. That makes sense.

@acelaya acelaya merged commit a502284 into main Nov 13, 2024
4 checks passed
@acelaya acelaya deleted the select-button-padding branch November 13, 2024 15:52
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.

None yet

2 participants