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

.h-* classes change color of anchor #36365

Closed
3 tasks done
WinterSilence opened this issue May 16, 2022 · 22 comments · Fixed by #36509
Closed
3 tasks done

.h-* classes change color of anchor #36365

WinterSilence opened this issue May 16, 2022 · 22 comments · Fixed by #36509

Comments

@WinterSilence
Copy link
Contributor

WinterSilence commented May 16, 2022

Prerequisites

Describe the issue

.h-* classes adds to <a> tags must keep anchor color.
Need fix like this:

h1, h2, h3, h4, h5, h6,
.h1, .h2, .h3, .h4, .h5, .h6 {
  margin-top: 0;
  margin-bottom: .5rem;
  font-weight: 500;
  line-height: 1.2;
}
h1, h2, h3, h4, h5, h6,
.h1:not(a), .h2:not(a), .h3:not(a), .h4:not(a), .h5:not(a), .h6:not(a) {
  color: var(--bs-heading-color);
}

Reduced test cases

<a href="#" class="h5">Link-title</a>
<a href="#" class="link-danger h5">Link-title</a>

What operating system(s) are you seeing the problem on?

Windows

What browser(s) are you seeing the problem on?

No response

What version of Bootstrap are you using?

5.2.0

@Kopyov
Copy link
Contributor

Kopyov commented May 16, 2022

Alternatively you can use

<h5>
    <a href="#" class="link-danger">Link title</a>
</h5>

where link color doesn't get overridden by heading color

@WinterSilence
Copy link
Contributor Author

@Kopyov I'm aware, captain Obvious

@Kopyov
Copy link
Contributor

Kopyov commented May 16, 2022

@WinterSilence So use it like this then. Following your suggestion and for the sake of consistency we would also need to re-use this logic in other elements such as dropdown items, nav links etc.

@mdo
Copy link
Member

mdo commented May 21, 2022

That's an interesting suggestion, but I don't think one we'll implement. It adds a good amount of specificity to what's otherwise a set of helpful utilities helpers.

@Kopyov I'm aware, captain Obvious

There's no need for name calling or attitude either, so please skip posting comments like that :).

@mdo mdo closed this as not planned Won't fix, can't repro, duplicate, stale May 21, 2022
@WinterSilence
Copy link
Contributor Author

@mdo Current implementation of .h* classes does not correspond to concept of utilities. Utilities changes something one: color, size, padding and yet.

@mdo
Copy link
Member

mdo commented May 21, 2022

Yes, my bad, they're helpers.

@WinterSilence
Copy link
Contributor Author

WinterSilence commented May 21, 2022

@mdo I don't think undocumented modification of existing helpers is a good idea, it's BC break for me

@mdo
Copy link
Member

mdo commented May 21, 2022

@mdo I don't think undocumented modification of existing helpers is a good idea, it's BC break for me

We haven't made any undocumented changes.

@WinterSilence
Copy link
Contributor Author

@mdo i can't find this changes in https://github.com/twbs/bootstrap/releases/tag/v5.2.0-beta1

@mdo
Copy link
Member

mdo commented May 21, 2022

There have been no changes to our heading classes in v5.2.0-beta1. I don't know what you're looking for or expecting to see.

@WinterSilence
Copy link
Contributor Author

WinterSilence commented May 21, 2022

@mdo
in 5.2:

.h1, .h2, .h3, .h4, .h5, .h6, h1, h2, h3, h4, h5, h6 {
  margin-top: 0;
  margin-bottom: .5rem;
  font-weight: 500;
  line-height: 1.2;
  color: var(--bs-heading-color);
}

in 5.1:

.h1, .h2, .h3, .h4, .h5, .h6, h1, h2, h3, h4, h5, h6 {
  margin-top: 0;
  margin-bottom: .5rem;
  font-weight: 500;
  line-height: 1.2;
}

https://codepen.io/WinterSilence/pen/KKQvKPQ?editors=1000

@WinterSilence
Copy link
Contributor Author

@mdo
Copy link
Member

mdo commented May 21, 2022

Sorry, I knew I should've been more specific. There's been no functional change to the headings in v5.2.0-beta1. Heading color has always been a null value, and now so is color: var(--bs-heading-color). Look at :root and you'll find --bs-heading-color: ;. There's no new color being set here, despite the addition of the CSS variable.

In v5.2.0-beta1, now with CSS variable:

%heading {
margin-top: 0; // 1
margin-bottom: $headings-margin-bottom;
font-family: $headings-font-family;
font-style: $headings-font-style;
font-weight: $headings-font-weight;
line-height: $headings-line-height;
color: var(--#{$prefix}heading-color);
}

And in the browser, you can see that it takes no effect because the variable is empty:

Screen Shot 2022-05-21 at 10 25 07 AM

In v5.1.3, with the Sass variable (also set to null in _variables.scss):

%heading {
margin-top: 0; // 1
margin-bottom: $headings-margin-bottom;
font-family: $headings-font-family;
font-style: $headings-font-style;
font-weight: $headings-font-weight;
line-height: $headings-line-height;
color: $headings-color;
}

Sass takes a property with null value and removes it from the compiled CSS. But that doesn't happen with CSS variables, they just get rendered out there. We could perhaps do a null CSS variable check and remove the property entirely, but that prevents the use case of CSS variables entirely.

@WinterSilence
Copy link
Contributor Author

@mdo as I'm already say, your changes style of <a class="h*">.

@WinterSilence
Copy link
Contributor Author

@mdo in 5.2:
изображение
in 5.1:
изображение

@mdo
Copy link
Member

mdo commented May 21, 2022

Ah, I see it now. Sorry for the back and forth—next time please include actual Codepen demos vs just snippets here :). Or include those kind of screenshots. I should've looked more closely though, too.

Let me reopen this and jump back into it for our stable release to see if there's a more logical fix than a slew of more specific :not() selectors.

@mdo mdo reopened this May 21, 2022
@WinterSilence
Copy link
Contributor Author

WinterSilence commented May 21, 2022

@mdo

next time please include actual Codepen demos vs just snippets here :). Or include those kind of screenshots. I should've looked more closely though, too.

Ok, no problem. Please, don't close issues, if you don't understand his reasons - ask details.

@mdo mdo added this to To do in v5.2.0-stable via automation Jun 1, 2022
v5.2.0-stable automation moved this from To do to Done Jul 11, 2022
@WinterSilence
Copy link
Contributor Author

@mdo 👍🏻

@UtkarshVerma
Copy link

I think this has surfaced again in v5.3.0. Could you please reopen this? @mdo

@julien-deramond
Copy link
Member

@UtkarshVerma Would you mind creating a new issue with a CodePen (or other) to show the problem? We tend not to reopen issues closed by a PR so that future selves are able to read easily the complete history and find stuff in the past.

@UtkarshVerma
Copy link

@julien-deramond Sure, here is the pen: https://codepen.io/UtkarshVerma/pen/MWzopyq

@maritto
Copy link

maritto commented Jul 29, 2023

are there plans to address this issue and if so could you share an ETA for it?
if there is already another issue open regarding this could you please link it here ? @mdo @julien-deramond

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-stable
  
Done
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants