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

Add top margin to nested module prefix in sidebar #1929

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

zachallaun
Copy link
Contributor

I was browsing some docs earlier that used the :nest_modules_by_prefix config and found that, with a very long module list, it was very difficult to see the prefixes and almost impossible to quickly scan for (e.g. while scrolling down to find the next section).

This PR just adds 10px of margin above those prefixes, which is sufficient to visually set them apart. (10px is chosen because it's the same amount used when you expand a module subheader in the sidebar.)

Before

image

After

image

@@ -245,6 +245,7 @@
font-size: 0.9em;
line-height: 1.8em;
color: var(--sidebarSubheadings);
margin-top: 10px;
Copy link
Member

Choose a reason for hiding this comment

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

The grouping functionality uses 1.5em. Should we use the same?

Suggested change
margin-top: 10px;
margin-top: 1.5em;

Copy link
Contributor Author

@zachallaun zachallaun Jul 2, 2024

Choose a reason for hiding this comment

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

I'm fine with that if you'd prefer. I personally think it errs on the side of too much margin, but I don't feel strongly. Here's what that looks like:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the grouping functionality: that font size is .8em whereas the module nesting uses .9em, so 1.5em margin ends up being a bit larger for module nesting. To make them consistent, we could update module nesting to use .8em and 1.5em margin.

Copy link
Contributor Author

@zachallaun zachallaun Jul 2, 2024

Choose a reason for hiding this comment

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

(Sorry for the repeated messages.) Looking at it now, bumping the nested modules down to .8em seems a bit too small. The other grouping uses ALL CAPS, so it's more readable at that smaller size.

I'll ultimately defer to you.

image

@josevalim josevalim merged commit dc8bc91 into elixir-lang:main Jul 2, 2024
5 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@zachallaun zachallaun deleted the za-nested-modules-style branch July 2, 2024 15:04
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.

None yet

2 participants