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

Rely on border-width for <hr> size #35491

Merged
merged 7 commits into from Feb 22, 2022
Merged

Rely on border-width for <hr> size #35491

merged 7 commits into from Feb 22, 2022

Conversation

ffoodd
Copy link
Member

@ffoodd ffoodd commented Dec 8, 2021

Fixes #34292, fixes #34374, fixes #35616.

A few tries confirmed me that relying on border-width as in v4 doesn't imply any side effect, even when using the (deprecated) size attribute.

I don't see any reason to stick with height then, so ditching this to make our new hr closer to v4.

Contrary to what we said in #34574, we can keep the opacity feature this way. With my current proposal, customizing hr could be done using .text-* for color and .border-* for sizing. I'd stick to currentColor for color since it allows inheritance: relying on border-color would require us to define a value (as it was done in v4) but I think it's more valuable to inherit from text color, isn't it?

NB: the current migration guide mentions v5.2.0, which is not accurate, is it? Do we maintain a minor versions' migration guide anywhere?

@ffoodd ffoodd requested a review from a team as a code owner December 8, 2021 16:24
@XhmikosR XhmikosR added this to In progress in v5.2.0 via automation Dec 9, 2021
@XhmikosR XhmikosR changed the title Rely on border-width for <hr> size Rely on border-width for <hr> size Dec 9, 2021
@XhmikosR XhmikosR changed the title Rely on border-width for <hr> size Rely on border-width for <hr> size Dec 9, 2021
@mdo
Copy link
Member

mdo commented Feb 9, 2022

So while this makes sense to me, it is a breaking change. <hr class="py-4"> would no longer work, nor would setting height on <hr>s. Do we have enough problems/complications with this currently to warrant undoing the v5 change?

@mdo mdo removed this from In progress in v5.2.0 Feb 9, 2022
@ffoodd
Copy link
Member Author

ffoodd commented Feb 9, 2022

AFAIC We're pretty fine here, but we already have two issues around <hr> with several comments. If we don't land this, we need to provide workarounds for the said issues (probably mixing utilities).

Does this sound better?

@mdo mdo added this to In progress in v5.2.0 via automation Feb 10, 2022
scss/_variables.scss Outdated Show resolved Hide resolved
Co-authored-by: Mark Otto <markd.otto@gmail.com>
v5.2.0 automation moved this from In progress to Reviewer approved Feb 10, 2022
@XhmikosR XhmikosR merged commit 37f3977 into main Feb 22, 2022
v5.2.0 automation moved this from Reviewer approved to Done Feb 22, 2022
@XhmikosR XhmikosR deleted the main-fod-hr branch February 22, 2022 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.2.0
  
Done
3 participants