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

Increase TOC spacing between entries (again) #1177

Merged
merged 3 commits into from
May 21, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented May 17, 2024

#1175 made the sidebar table of contents less dense, but it still looks too dense. It's hard to distinguish between what is a different entry or just an entry with a name that spans multiple lines, and it just looks a bit out of place with the rest of the layout being more spacious.

Before:

kuva

After:

kuva

Note: The screenshots above use the text styling from #1176, but these changes are not included in this PR.

@doup
Copy link
Contributor

doup commented May 17, 2024

Hahaha, again. For me it's a 4. Two 5px rows won't add to a multiple of 4px (not an argument really unless you self-impose this arbitrary "4px multiples" rule to yourself), 6px adds too much white space.

@doup
Copy link
Contributor

doup commented May 18, 2024

I just noticed, in favor of 4px: is what the sidebar .tree-menu__link uses. 🙃

Copy link
Contributor

@SIGSTACKFAULT SIGSTACKFAULT left a comment

Choose a reason for hiding this comment

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

Need to remove the rule on line 42, or else the spacing would be smaller on mobile. Or make it even bigger, up to you.

@Jondolf
Copy link
Contributor Author

Jondolf commented May 18, 2024

Need to remove the rule on line 42, or else the spacing would be smaller on mobile. Or make it even bigger, up to you.

Oops, missed that. I made it 6px for now since 4px definitely seemed too small on mobile and even 5px was a bit small. (And based on the existing comment it should be larger than desktop, unless we just remove the rule)

I just noticed, in favor of 4px: is what the sidebar .tree-menu__link uses. 🙃

It uses that, yes, but the padding for that doesn't actually affect the vertical size (except for multi-line items) because the height there is determined by min-height: 32px. With both using a vertical padding of 4px (for top and bottom), the tree menu links are 32px high while the TOC items are just 26.67px high. So the visual spacing is different anyway. With a padding of 6px or even 7px the height would be more similar (not that we necessarily want it to be the exact same).

Here's one more comparison of the options:

4px:
kuva

5px:
kuva

6px:
kuva

I would personally go for either 5px or 6px, both are fine for me. I generally prefer making spacing larger unless there is practical benefit to making it more dense (like as an option for email or messaging applications).

@Jondolf
Copy link
Contributor Author

Jondolf commented May 18, 2024

We could also use padding-block: 4px, but make the min-height larger similarly to the tree menu links

Copy link
Contributor

@doup doup left a comment

Choose a reason for hiding this comment

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

I was going to give the OK to 5px… but now that I see this after few days, 6px also doesn't look bad. Either one is OK for me. 😅

@Jondolf Jondolf added S-Ready-For-Final-Review Ready for a maintainer to consider for merging and removed S-Needs-Review labels May 20, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 21, 2024
Merged via the queue into bevyengine:main with commit a2768c1 May 21, 2024
10 checks passed
@Jondolf Jondolf deleted the less-dense-tree-menu-toc branch May 21, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Book A-Migration Guides A-Quick-Start About the Quick Start Guide C-Webdev S-Ready-For-Final-Review Ready for a maintainer to consider for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants