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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 1, 2025
Merged

Polish link underline in Darkfish CSS #1349

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9c0a65
Polish link underline in Darkfish CSS
ybiquitous Apr 28, 2025
f730435
Increase `text-underline-offset` value with `em` unit
ybiquitous Apr 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/rdoc/generator/template/darkfish/css/rdoc.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tompng @vinistock I've pushed the commit f730435. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good!

text-underline-offset: 0.2em; /* Make sure it doesn't overlap with underscores in a method name. */
}

a:hover {
Expand Down