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

Add check to rgba-css-var function for body or bg #34699

Merged
merged 1 commit into from
Aug 10, 2021
Merged

Conversation

mdo
Copy link
Member

@mdo mdo commented Aug 5, 2021

This is the most straightforward fix I can think of for this situation. It's definitely a my bad for not thinking about the new CSS var --body-rgb and there needing to be two separate CSS vars. This PR drops the --bs-body-rgb for two separate ones (so it's a small breaking change), but keeps the .bg-body and .text-body intact without change. it also required duplicating the utilities maps a bit—so we can differentiate at the Sass map level between bg and color. I think this is an acceptable solution, but definitely want to hear thoughts.

Fixes #34688.

/cc @ffoodd

@mdo mdo requested a review from a team as a code owner August 5, 2021 21:04
@mdo mdo added this to In progress in v5.1.1 via automation Aug 5, 2021
@mdo
Copy link
Member Author

mdo commented Aug 5, 2021

Test failure seems unrelated, will re-run to see if I can get it to pass.

v5.1.1 automation moved this from In progress to Reviewer approved Aug 6, 2021
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

This works fine, and I can't find a better way through ATM 😄

@XhmikosR XhmikosR changed the title Add check to rgba-css-var function for body or bg Add check to rgba-css-var function for body or bg Aug 10, 2021
@XhmikosR XhmikosR merged commit 612d235 into main Aug 10, 2021
v5.1.1 automation moved this from Reviewer approved to Done Aug 10, 2021
@XhmikosR XhmikosR deleted the fix-body-vars-utils branch August 10, 2021 14:16
marvin-hinkley-vortx pushed a commit to Vortx-Inc/bootstrap that referenced this pull request Aug 18, 2021
@oliwerAR
Copy link

I've just stumbled on the .body-bg problem after update. But the proposed fix seems be a bit messy. Instead of messing with the rgba-css-var function and hardcoding and exepction, I think it would be better to leave rgba-css-var as it was before and use a consistent pattern in vaiables:

$utilities-text: map-merge(
  $utilities-colors,
  (
    "black": to-rgb($black),
    "white": to-rgb($white),
    "body-color": to-rgb($body-color)
  )
) !default;
$utilities-bg: map-merge(
  $utilities-colors,
  (
    "black": to-rgb($black),
    "white": to-rgb($white),
    "body-bg": to-rgb($body-bg)
  )
) !default;

It would be easier to understand what's going - the CSS var naming in particular.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.1.1
  
Done
Development

Successfully merging this pull request may close these issues.

Docs: background color example for bg-body has invisible text
4 participants