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 $enable-important-utilities condition in colored links #37252

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Oct 3, 2022

Description

As mentioned in the issue !important was set whatever if $enable-important-utilities was true or false for colored links. This PR adds a new condition as it is already done for other utilities.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • All new and existing tests passed

How to test

Check with/without enabling $enable-important-utilities to run npm run css.

When enabled:

.link-primary {
  color: #0d6efd !important;
}
.link-primary:hover, .link-primary:focus {
  color: #0a58ca !important;
}

When disabled:

.link-primary {
  color: #0d6efd;
}
.link-primary:hover, .link-primary:focus {
  color: #0a58ca;
}

Related issues

Closes #37251

@julien-deramond julien-deramond force-pushed the main-jd-colored-links-enable-important branch from f675190 to bd0b814 Compare October 3, 2022 19:46
@mdo
Copy link
Member

mdo commented Oct 5, 2022

I get why we should do this one, but helpers aren't utilities and this Sass variable is primarily for utilities. Thoughts?

@julien-deramond
Copy link
Member Author

julien-deramond commented Oct 5, 2022

I get why we should do this one, but helpers aren't utilities and this Sass variable is primarily for utilities. Thoughts?

Atm it is done like this in _color-bg.scss as well. But you're right. Since the target release is the v5.3.0, we could create a $enable-important-helpers to use there and explain in the migration guide or somewhere else that something has changed for .text-bg-*. What do you think of this proposal?

@mdo
Copy link
Member

mdo commented Oct 6, 2022

Meh, let's roll with this for now :). We can revisit later if needed.

@mdo mdo merged commit 9936ed4 into main Oct 6, 2022
@mdo mdo deleted the main-jd-colored-links-enable-important branch October 6, 2022 20:04
@narbehm
Copy link

narbehm commented Apr 4, 2024

I get why we should do this one, but helpers aren't utilities and this Sass variable is primarily for utilities. Thoughts?

Atm it is done like this in _color-bg.scss as well. But you're right. Since the target release is the v5.3.0, we could create a $enable-important-helpers to use there and explain in the migration guide or somewhere else that something has changed for .text-bg-*. What do you think of this proposal?

Maybe the time is now? The default behavior of !important for link colors breaks many use cases where the default classes are overridden in some custom theme. Unfortunately setting $enable-important-utilities to false affects other behavior that we don't want. Being able to turn this feature off only for colored links would be a plus.

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

Successfully merging this pull request may close these issues.

$enable-important-utilities condition missing in _colored-links.scss
3 participants