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

v5: Underline links #30389

Merged
merged 7 commits into from Mar 20, 2020
Merged

v5: Underline links #30389

merged 7 commits into from Mar 20, 2020

Conversation

mdo
Copy link
Member

@mdo mdo commented Mar 12, 2020

This PR changes the $link-text-decoration from none to underline to help improve link discovery. It also includes a fix to the docs sidebar to override that for our desired look. I've kind of missed underlines on our links throughout the docs at least, so this feels like a win-win to me.

With the work done in #30262, this also doesn't affect any existing Bootstrap component. All the components you'd expect will nullify the underline thanks to the changes from that PR.

Fixes #15304.


Let me know your thoughts @patrickhlauke and @twbs/css-review.

Preview: https://deploy-preview-30389--twbs-bootstrap.netlify.com/

@MartijnCuppens
Copy link
Member

The hover text-decorations covered in https://github.com/twbs/bootstrap/pull/30262/files can now be nullified if $link-hover-decoration == null

@XhmikosR
Copy link
Member

Should we remove the effect for ToC links?

Generally I can't tell if I like it or not, I guess it takes some time to evaluate this since I was used to no underline.

@MartijnCuppens
Copy link
Member

Should we remove the effect for ToC links?

They are going to rethemed anyway: #30354

Generally I can't tell if I like it or not, I guess it takes some time to evaluate this since I was used to no underline.

You'll get used to it. It's way clearer for visually impared people.

@patrickhlauke
Copy link
Member

from a cursory glance on the netlify preview...i like it (but yes should be rethemed/suppressed for the TOCs)

@ysds
Copy link
Member

ysds commented Mar 13, 2020

I agree with @XhmikosR, but anyway, we have time before the official v5 release, so let's go ahead with this and get feedback from users.

@mdo mdo requested a review from a team March 20, 2020 03:30
@mdo
Copy link
Member Author

mdo commented Mar 20, 2020

Updated this the other day so it's ready to go I think.

@XhmikosR
Copy link
Member

I think we should wait a bit on this. For example, I see text-muted has no visual difference when focusing or hovering. Not sure if it's intentional. Also, there might be more such cases.

@MartijnCuppens
Copy link
Member

I've pushed some changes I mentioned in #30389 (comment).

I see text-muted has no visual difference when focusing or hovering.

text-muted shouldn't have a hover state, the colored links should be used for this.

We might need to revisit the color change of hovered links later, since they are always darkened. See

color: darken($value, $emphasized-link-hover-darken-percentage);

v5 automation moved this from Inbox to Approved Mar 20, 2020
@XhmikosR
Copy link
Member

text-muted shouldn't have a hover state, the colored links should be used for this.

We need to fix this in our docs then. I find it wrong that in the homepage, where we use text-muted, there's no focus/hover style.

@patrickhlauke
Copy link
Member

I find it wrong that in the homepage, where we use text-muted, there's no focus/hover style.

for focus, we at least have the default outline though, just to clarify.

Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

not checked all possible variations/edge cases, but this looks good to me from looking over the deploy preview

@XhmikosR
Copy link
Member

for focus, we at least have the default outline though, just to clarify.

True, TBH I didn't even notice it :P

Anyway, I'm not blocking the patch :)

@MartijnCuppens
Copy link
Member

MartijnCuppens commented Mar 20, 2020

I'll open a new PR to tackle the hover color of the link on the homepage, we can merge this if all checks succeed (see #30427)

@patrickhlauke
Copy link
Member

patrickhlauke commented Mar 20, 2020

I'll open a new PR to tackle the hover color of the link on the homepage

be good to check that various situations (are there any more, beyond .text-muted?) are also covered in general

(ah, looking at that PR, i see in THIS case we added .text-muted directly to the link. fair enough, for that the fix is correct there. but yes fundamentally we should probably look at more explicit hover styles for runs of text that is set to .text-muted - and anything similar - that contain links)

@mdo mdo merged commit 0388ee0 into master Mar 20, 2020
v5 automation moved this from Approved to Shipped Mar 20, 2020
@mdo mdo deleted the underline-links branch March 20, 2020 15:58
wiwiEnnis pushed a commit to pnetwork/bootstrap that referenced this pull request Sep 25, 2020
v5: Underline links
# Conflicts:
#	scss/_nav.scss
#	scss/_variables.scss
#	site/assets/scss/_sidebar.scss
#	site/assets/scss/_toc.scss
froboy added a commit to froboy/docsy that referenced this pull request Oct 2, 2023
As per the change upstream in Bootstrap 5 twbs/bootstrap#30389
chalin pushed a commit to froboy/docsy that referenced this pull request Feb 2, 2024
As per the change upstream in Bootstrap 5 twbs/bootstrap#30389
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

Links (as part of regular text) not distinguishable enough
5 participants