-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
nixos-render-docs: make examples collapsible by default #301152
Conversation
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.
Looks good to me. Seems we have highly specific css.
@@ -347,6 +347,22 @@ div.appendix div.example { | |||
margin-top: 1.5em; | |||
} | |||
|
|||
div.book div.example details, | |||
div.appendix div.example details { |
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.
Your CSS assumes a very specific HTML structure. If the HTML changes, such as adding another parent div this will break.
Id suggest '.example details'
which targets all details with a parent of class .example
this would allow other parents as well. making it more resistent to potential future changes.
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 prefer to keep the specific CSS here - it's only used with the output of nrd, and we control the HTML structure there. It's expected that changes to nrd will be at least rendered so we can glance over the output, and if the CSS needs to be adapted (such as in this case), then we'll just change the CSS as well.
To explain why I'm taking this stance: this CSS file has a few rules that conflict on certain elements, and we rely on the more specific rule overriding the value of the less specific one.
As an exercise, I tried removing all the div.book
and div.appendix
specifiers from all the other rules as well, and some things got messed up.
I prefer keeping the specific rules because that's how the rest of the CSS is laid out, so it avoids subtle things from breaking if someone decides to change some other less-specific rule.
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.
(reviewed in the docs team)
Looking good, this is awesome, let's merge!
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-04-04-documentation-team-meeting-notes-117/42754/1 |
Description of changes
During work on #297654 it got a bit annoying IMO to always see examples fully shown by default on the manuals. This is especially annoying when we want to add an example somewhere in the middle of some explanation, when there's no better place to move the example to (and then link to it from the text).
This PR wraps each example under a
<details>
tag which makes every example collapsible, and each one is collapsed by default. It also adds a bit of CSS to make it more usable.An example of what it looks like on my browser:
And after expanding an example:
The CSS is up for discussion. I tried with and without the borders after expanding, but eventually settled on borders because some examples are kinda large, and the borders help determine when an example ends. I could definitely use help improving the visual though.
Things I tested:
id
attached to them works.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.