-
Notifications
You must be signed in to change notification settings - Fork 30
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
fix(Anchor:) anchor modifier .dnb-anchor--no-hover #2536
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 3ecae36:
|
33f29b7
to
feda4c2
Compare
feda4c2
to
715f800
Compare
715f800
to
53dc72a
Compare
53dc72a
to
476517d
Compare
@@ -19,6 +13,11 @@ | |||
color: var(--color-sea-green); | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hover needs to be before active, if not, I think the active style will never actually work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it wouldn't help in this case. The hover rule has higher specificity than the active rule because of the :not
preudo-classes. So adding the :not
rules on the hover ruins active. The only way would be to also increase the specificity of the :active rule. Something like:
&:active {
&, &:not(.dnb-anchor--no-hover):not(.dnb-anchor--no-style) {
color: var(--color-mint-green);
@include anchor-mixins.anchorBackground(var(--color-emerald-green));
}
}
}
&:hover:not(.dnb-anchor--no-hover):not(.dnb-anchor--no-style) {
color: var(--color-sea-green);
@include anchor-mixins.anchorBackground(var(--color-mint-green-50));
}
But honestly, increasing specificity keeps causing issues down the line. Especially with theming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can also try a solution using css variables?
&:not(.dnb-anchor--no-hover):not(.dnb-anchor--no-style) {
--h-color: blue;
}
&:hover {
color: var(--h-color);
}
&:active {
color: red;
}
but that really doesn't scale well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, and now that we can use runtime variables, we can do that. It eliminates specificity issues. But it at least needs an extensive rewrite.
What do you think about when you write that it does not scale well? Because of later complexity? Or because it will just solve some issues, but not other needs (and make them more complex)?
We may wait with this PR until PR-2521 is merged. |
It's been merged |
2735a20
to
8b972b6
Compare
8b972b6
to
4abeed0
Compare
96a56dd
to
cc78432
Compare
cc78432
to
74f782b
Compare
74f782b
to
8598f08
Compare
8598f08
to
1a60fe9
Compare
1a60fe9
to
df29677
Compare
af21ec1
to
5026051
Compare
5026051
to
3f7ec0d
Compare
I've gotten quite stuck on this, so what I did now was to first generate screenshot tests for classes Also changed the PR back to draft, as there's stuff that needs to be improved 👨💻 Whatever approach I've tried implementing, I always end up with multiple unexpected collisions with hover effects of different variants(contrast), and other themes... |
3f7ec0d
to
a9363f0
Compare
a9363f0
to
3ecae36
Compare
Summary
As of today we offer a few "anchor modifier classes", see https://eufemia.dnb.no/uilib/components/anchor/demos/#anchor-modifiers. Two of them,
.dnb-anchor--no-hover
, and.dnb-anchor--no-style
(which is supposed to have no style and no hover) doesn't seem to work as expected, see the following video:Screen.Recording.2023-07-28.at.12.03.06.mov
This PR aims to fix these two modifiers
.dnb-anchor--no-hover
, and.dnb-anchor--no-style
, to not have any hover effect/styling when hovering. This is the result after this PR:Screen.Recording.2023-07-28.at.12.03.32.mov
I'm however not 100% sure how to best fix this, and what I've done now is just an alternative.
In our codebase
:hover
styling for anchor gets applied multiple places, in@mixin anchorStyle()
in bothanchor-mixins.scss
anddnb-anchor-theme-ui.scss
.And to be able to get a correct look and behaviour for the anchor without hover, it's now dependent on much stuff to make it happen:
.dnb-anchor--no-hover
inanchor-mixins.scss
which sets some CSS properties(background, not sure if this is needed?).&:hover:not(.dnb-anchor--no-hover):not(.dnb-anchor--no-style) {
inanchor-mixins.scss
.&:hover:not(.dnb-anchor--no-hover):not(.dnb-anchor--no-style) {
indnb-anchor-theme-ui.scss
.I thought about extending/adding more to the
@mixin resetAnchorHoverStyle()
instead, but I'm not confident that overriding all the styles that gets added to:hover
in this mixin would lead to a better result 🤔Hence I rather avoid adding
:hover
styles when.dnb-anchor--no-hover
and.dnb-anchor--no-style
.I've also had to change the order of the selectors because of stylelint's
no-descending-specificity
.