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

Iterate on border utilities #36239

Merged
merged 3 commits into from
May 6, 2022
Merged

Iterate on border utilities #36239

merged 3 commits into from
May 6, 2022

Conversation

mdo
Copy link
Member

@mdo mdo commented Apr 28, 2022

We set --bs-border-opacity: 1 globally at the :root level, so redeclaring it on every .border-* utility doesn't make much sense. I think we can drop this.

Edit: Per conversation below, I've also removed the global --bs-border-opacity and restored it on the .border-{color} utilities. There's no need for border opacity on default .border utilities, just the color ones, which now properly matches our .text-{color} and .bg-{color} utilities. In addition, this rearranged the utilities map to put border-color at the end (without which .border-opacity-* classes wouldn't work properly.

@mdo mdo requested a review from a team as a code owner April 28, 2022 00:59
@julien-deramond
Copy link
Member

julien-deramond commented Apr 28, 2022

Tested it in border utilities and some components. Indeed it seems safe to drop this 👌
However, Utilities > Borders > Opacity > How it works would probably need an update:

  • Drop --bs-border-opacity: 1; in the code shown for .border-success since it is defined in :root.
  • Modify the description just after which mentions "(with a default value 1 thanks to a local CSS variable)".

@mdo
Copy link
Member Author

mdo commented Apr 28, 2022

@julien-deramond Should we do the global value, or use a local one? We use local ones on our .bg-* and .text-* utilities... should we maybe restore the local and remove the global, so they all match?

@julien-deramond
Copy link
Member

Yes indeed you could restore the local and remove the global so they all match.

If I am remembering well 🤔 there are some edge cases in https://twbs-bootstrap.netlify.app/docs/5.1/utilities/borders/#additive where you can add .border-danger to .border-top, .border-bottom, .border-bottom-0, etc. (without having to add an extra .border).
After removing the global, maybe you will need to add the following:

"border-color": (
  // ...
  local-vars: (
    "border-opacity": 1
  ),
  // ...
),

@mdo mdo changed the title Remove --bs-border-opacity: 1 from .border-* utilities Iterate on border utilities May 2, 2022
@mdo mdo force-pushed the rm-border-opacity-from-utils branch from a5be314 to f78c0d9 Compare May 2, 2022 04:06
@mdo
Copy link
Member Author

mdo commented May 2, 2022

Good eyes @julien-deramond. I think the latest commit addresses all concerns. Played around with mixing the utilities and using them on components.

@julien-deramond
Copy link
Member

julien-deramond commented May 2, 2022

The latest commit addresses all concerns. Also played around with mixing the utilities and using them on components and didn't see any regressions. I "approve" this PR :)

By the way it would be very interesting to have the .border-*-0 (or .border-{top|end|etc.}) + .border-{color} combos automatically visually tested (or with components as well) to avoid any regressions. IMO it shouldn't be in the documentation so having them automatically tested would offer security; we won't remember those edge cases in few weeks or months.

mdo and others added 3 commits May 5, 2022 21:07
We set `--bs-border-opacity: 1` globally at the `:root` level, so redeclaring it on every `.border-*` utility doesn't make much sense. I think we can drop this.
…ove .border-color utils down the list to fix some specificity issues
@mdo
Copy link
Member Author

mdo commented May 6, 2022

Added some examples @julien-deramond:

Screen Shot 2022-05-05 at 9 17 07 PM

Also removed the callout that was now out of date regarding the setting of CSS variables only for utilities. We backtracked from that on .text-* and .bg-* awhile back for backward compatibility. In v6, we'll make these utilities use CSS vars only I believe. More to test on that though when the time comes.

@mdo mdo force-pushed the rm-border-opacity-from-utils branch from f78c0d9 to c80bbb2 Compare May 6, 2022 04:19
@mdo mdo merged commit bca9923 into main May 6, 2022
@mdo mdo deleted the rm-border-opacity-from-utils branch May 6, 2022 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants