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.

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

Jump to bottom

Improve theme support when JS is disabled #2454

Merged
merged 1 commit into from
Nov 2, 2024
Merged

Improve theme support when JS is disabled #2454

merged 1 commit into from
Nov 2, 2024

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Oct 23, 2024 •
edited
Loading

Little PR to follow the system's light/dark theme when JS is disabled.

@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Oct 23, 2024
@GuillaumeGomez GuillaumeGomez force-pushed the theme-noscript branch 3 times, most recently from c3c6db2 to edcf05e Compare October 23, 2024 19:59
@GuillaumeGomez
Copy link
Member Author

Finally fixed CI. ^^'

@ehuss
Copy link
Contributor

ehuss commented Oct 23, 2024

@notriddle Is this something you'd be willing to help review? No worries if not, I don't want to pressure anyone.

@GuillaumeGomez
Copy link
Member Author

I think it'd be nice for mdBook to get more reviewers, so if needed, don't hesitate to ask me too for other PRs.

notriddle
notriddle suggested changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@notriddle notriddle left a comment

Choose a reason for hiding this comment

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

It looks like it's just code cleanup without any user-visible change, but your commit message sounds like it's saying that themes actually work better in noscript.

I don't think this PR allows you to actually switch themes when JS is disabled (I don't know how you'd even do that). It does allow you to see the menu bar, but the theme button isn't there.

The code itself seems okay, other than one question I asked here.

src/theme/css/noscript.css Outdated Show resolved Hide resolved
src/theme/index.hbs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

It looks like it's just code cleanup without any user-visible change, but your commit message sounds like it's saying that themes actually work better in noscript.

It's now taking into account the system theme, which was not the case until now. So it is an improvement. ;)

I don't think this PR allows you to actually switch themes when JS is disabled (I don't know how you'd even do that). It does allow you to see the menu bar, but the theme button isn't there.

It doesn't indeed.

@notriddle
Copy link
Contributor

It's now taking into account the system theme, which was not the case until now. So it is an improvement. ;)

Okay. Please say that in the PR description. Instead of just "Little PR to improve theme support when JS is disabled.", I would prefer something like "Little PR to follow the system's light/dark theme when JS is disabled."

@GuillaumeGomez
Copy link
Member Author

Good idea, done!

@ehuss
Copy link
Contributor

ehuss commented Oct 26, 2024

This seems like quite a large change compared to #2441. Why is this approach preferred over the one in #2441?

@GuillaumeGomez
Copy link
Member Author

This PR removes some JS (which is why there are more changes, removing all .no-js items in CSS for example), introduces noscript.css which allows to have style specifically when JS is disabled.

Overall this PR is more complete I'd say.

notriddle
notriddle reviewed Oct 26, 2024
View reviewed changes
src/theme/css/chrome.css
@@ -107,7 +104,7 @@ a > .hljs {
overflow: hidden;
text-overflow: ellipsis;
}
.js .menu-title {
.menu-title {
cursor: pointer;
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand it correctly, this change will result in the 👆 cursor being used on the menu title, even when javascript is disabled. I don't think clicking on the title actually does anything when javascript is disabled, so you'd need to do something to prevent that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. We could emulate the same behaviour (scrolling to the top) without JS by replacing this element with <a href="#">...</a>. Should I do it in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that should be a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

src/theme/css/noscript.css Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Removed the noscript.css file. Diff is much smaller now.

@GuillaumeGomez
Copy link
Member Author

Applied your suggestions. :)

notriddle
notriddle suggested changes Oct 28, 2024
View reviewed changes
src/theme/css/chrome.css Outdated Show resolved Hide resolved
src/theme/css/chrome.css Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Updated!

@GuillaumeGomez
Copy link
Member Author

@ehuss PR is now ready. :)

ehuss
ehuss approved these changes Nov 2, 2024
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss added this pull request to the merge queue Nov 2, 2024
This was referenced Nov 2, 2024
Closed
Closed
Merged via the queue into rust-lang:master with commit 5ec4f65 Nov 2, 2024
11 checks passed
@GuillaumeGomez GuillaumeGomez deleted the theme-noscript branch November 3, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notriddle notriddle notriddle approved these changes

@ehuss ehuss ehuss approved these changes

Assignees
No one assigned
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GuillaumeGomez @ehuss @notriddle @rustbot