Skip to content

Polish link underline in Darkfish CSS #1349

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ybiquitous
Copy link
Contributor

This aims to make sure an underline in a link doesn't overlap with underscores in a method name.

For example, method names on the following page seem a bit hard to read because underscores in a method name are hidden in an underline:
image
https://ruby.github.io/rdoc/table_of_contents.html#methods

This PR improves it a bit:
image

Note: The text-underline-offset CSS property is widely available now.

This aims to make sure an underline in a link doesn't overlap with underscores in a method name.
@@ -96,6 +96,8 @@ main .anchor-link:target {
a {
color: var(--link-color);
transition: color 0.3s ease;
text-decoration: underline;
text-underline-offset: 2px; /* Make sure it doesn't overlap with underscores in a method name. */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[note] Why not more spaces like 3px? If it becomes 3px, an inline code with a link looks like (this example is on the LEGAL page): I'm a bit concerned that the underline sticks to the bottom line of the inline code box.
image

In the case of 2px:
image

But if people are in favor of 3px or more, I'm glad to change it. 😃

Copy link
Member

Choose a reason for hiding this comment

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

I prefer 3px to 2px but I'm not good at visual design...

Others should decide it...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a design expert either, but I find the 3px version better.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@@ -96,6 +96,8 @@ main .anchor-link:target {
a {
color: var(--link-color);
transition: color 0.3s ease;
text-decoration: underline;
text-underline-offset: 2px; /* Make sure it doesn't overlap with underscores in a method name. */
Copy link
Member

Choose a reason for hiding this comment

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

I prefer 3px to 2px but I'm not good at visual design...

Others should decide it...

@@ -96,6 +96,8 @@ main .anchor-link:target {
a {
color: var(--link-color);
transition: color 0.3s ease;
text-decoration: underline;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor Author

@ybiquitous ybiquitous Apr 28, 2025

Choose a reason for hiding this comment

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

text-decoration: underline isn't necessarily required, but most browsers have it in their default user agent stylesheets. This is an example of Chrome:
image

So, I've added it to prevent our confusion, e.g., "Why is text-underline-offset specified, regardless text-decoration is not?"

If we want to respect browsers that don't have text-decoration: underline by default, we can remove it (text-underline-offset is just ignored). 👌🏼 In this case, it might be helpful to add a code comment that describes the reason for not having text-decoration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a question. We're adding this underline offset to all links, but not every link is a method name.

Will this look okay for other links that aren't method names? Should we use a more specific selector to match just method names or is the visual consistency better?

Copy link
Member

Choose a reason for hiding this comment

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

I think underline offset is fine for every links.
Link text could be an URL or a filename that contains underscore. Appearance of these links also gets better.

Another example, GitHub's "Convert to draft" link and URL in pull request description text also has text-underline-offset: .2rem; // about 3.2px.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. For your information, it's easy to restrict only links that have <code>, but it might be harder for other links, e.g.

<a href="..."><code>::check_modeline</code></a>

<a href="...">::check_modeline</a>
/* This is applied into the first `<a>` only, not the second one. */
a:has(code) { text-underline-offset: 3px; }

Copy link
Contributor Author

@ybiquitous ybiquitous Apr 29, 2025

Choose a reason for hiding this comment

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

As GitHub does, we can also use the rem unit instead of px. rem has a relative value from the root font size, so it might be better to maintain.

Considering the favors on #1349 (comment), I'll adjust the value if no other concerns:

  • Use rem instead of px
  • Increase the offset value from 2px to keep more space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've increased the text-underline-offset value from 2px to 0.2em via f730435. See the following screenshots to see the new value:

  • image
  • image

Indeed, 0.2em is interpreted as 3.2px since the <main> and <nav> have font-size: 16px (16px * 0.2 == 3.2px):

The reason that I chose the em unit instead of rem is because the Darkfish CSS doesn't specify font-size to :root or html elements (rem stands for "root em"). If this CSS has font-size for :root in the future, we may need to reconsider the em value at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants