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

[css] De-jumble header items at intermediate widths :) #1784

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sneakers-the-rat
Copy link

Hello :):)

I've noticed this on a few different sites that use pydata-sphinx-theme - the header items can look a little jumbly at intermediate widths where the navbar items need to wrap, eg. the current dev docs:

Screenshot 2024-04-24 at 7 41 19 PM

It just looks a little random - a lot of whitespace is wasted in the center, and the way that items wrap on the right is odd.

Here's a tiny little change to the CSS that changes the wrapping behavior and makes it look like this:

Screenshot 2024-04-24 at 7 36 50 PM

Specifically, i disabled wrapping in the top-level containers .navbar-header-items__end et al. instead the lower-level navbar-items can wrap, which works with the way that the template currently puts lists of links inside of them.

I also replaced the implicit vertical padding that came from giving items a fixed height to explicit padding in the outermost nav container.

The theme puts >5 header links in the More dropdown, but this still works in the case that the 5 displayed links are extremely long like this:
Screenshot 2024-04-24 at 7 32 24 PM

Ideally, rather than wrap, the search bar and items would just be hidden, but that would require JS since the CSS-only approach of setting a fixed height to the bar and hiding overflow wouldn't work here because the More menu would be cut off too. This is a very slight improvement imo that just makes things look a little tidier without any functional changes :)

@sneakers-the-rat sneakers-the-rat changed the title [css] De-jumble header items :) [css] De-jumble header items at intermediate widths :) Apr 25, 2024
@drammock
Copy link
Collaborator

Thanks for the contribution! Agree that this is an improvement. I can imagine some alternative approaches that might look better for our specific docs site (separately flex-wrapping the groups of __center and __end items?), but I'm not sure how generalizable they'll be to other sites. @gabalafou what do you think of the approach in this PR?

@trallard
Copy link
Collaborator

trallard commented May 7, 2024

@gabalafou bringing this to your attention since we have discussed navbar stuff too!

@Carreau
Copy link
Collaborator

Carreau commented May 30, 2024

Conflict fixed.

@gabalafou
Copy link
Collaborator

This is on my radar. Variable layout is hard. It's going to take me some time to review this but hopefully I can tackle this in the coming week.

@gabalafou
Copy link
Collaborator

I'm kinda hesitant to change the any of the layout properties without a strong design vision for this, which combines both how much customization we want to support and what constraints we want to enforce on the header nav layout

@sneakers-the-rat
Copy link
Author

Up to you. I dont have any design vision here except "make sensible use of the space," "avoid wrapping text in tiny boxes," and "keep semantically related things in visually distinct groups." Its an extremely minor change that fixes something that looks imo bad on a bunch of deployments of the skin that ive seen, but idrc if yall want to close this without some design statement :)

@gabalafou
Copy link
Collaborator

gabalafou commented May 30, 2024

I appreciate you trying to fix it! It's just that this is probably the hardest CSS on the site to get right. It's also a difficult design challenge, since the theme has to accommodate a number of different headers on different sites and still look good.

To make this more concrete, I'm not really sure that I agree that the second screenshot is an improvement over the first.

Here's why I think that. My understanding is that the brain is wired for spatial mappings. For sighted users, if the search button is on the right side of the page on both a wide desktop and a mobile screen, I don't think it would be a good idea to put the search button on the left side of the page on a medium-wide screen. It might look better visually, but I think it will upset users' expectations.

I totally agree with you that the current implementation needs improvement. It's just not clear to me at the moment how we should improve it.

@gabalafou
Copy link
Collaborator

Since this is in my headspace and I had an idea, I'm going to jot it down before I forget it.

I think my preferred design direction for this would be...

We constrain the header nav to one line. No wrapping. As soon as we start running out of space, first we compactify the search button, then start popping groups of items off the edges in a defined order into the hamburger menus.

Defined order would probably have to be:

  1. First pop the icon links together (twitter, github, etc.) into the right hamburger menu (currently these end up in the left hamburger menu, which I think is a mistake)
  2. Light/dark button -> right
  3. Version switcher + header nav links -> left

I'm not quite sure how much work this would be though, nor if constraining the header nav to a single line is feasible or too much work or truly desirable.

@gabalafou
Copy link
Collaborator

nor if constraining the header nav to a single line is feasible or too much work or truly desirable.

To elaborate... some of the sites that adopt the theme might need to display four or five links with long titles in the header nav. They might prefer to have the header nav height expand rather than have those header nav links moved into the left hamburger menu. See for example the third screenshot in the PR description, the one with the long links.

@drammock
Copy link
Collaborator

See for example the third screenshot in the PR description, the one with the long links.

this site (from one of our regular bug-reporters) would be a good test-bed for any changes; it has 8 header nav items and several of them are fairly long.

@dbitouze
Copy link
Contributor

this site (from one of our regular bug-reporters) would be a good test-bed for any changes; it has 8 header nav items and several of them are fairly long.

It would be a pleasure! 😃

@sneakers-the-rat
Copy link
Author

sneakers-the-rat commented May 31, 2024

For sighted users, if the search button is on the right side of the page on both a wide desktop and a mobile screen, I don't think it would be a good idea to put the search button on the left side of the page on a medium-wide screen. It might look better visually, but I think it will upset users' expectations.

yep. spent awhile trying to do that, and so:

Ideally, rather than wrap, the search bar and items would just be hidden, but that would require JS since the CSS-only approach of setting a fixed height to the bar and hiding overflow wouldn't work here because the More menu would be cut off too.

because the issue is need for real estate, so would either need to swap out to a search icon you could expand the search bar from or hide it entirely, but was trying to make a minimal PR. if you don't think it's an improvement then close this, bc i see it as an unambiguous improvement even if it's not the ideal.

As soon as we start running out of space, first we compactify the search button, then start popping groups of items off the edges in a defined order into the hamburger menus.

sure, again requiring JS intervention and lots of breakpoints or size checking. this is specifically an intermediate screen width issue where wrapping and >1.5em is fine, mobile is already one line. from what i saw the 5 links "more" collapse is hardcoded at generation time so would need a rewrite of that too.

this site (from one of our regular bug-reporters) would be a good test-bed for any changes; it has 8 header nav items and several of them are fairly long.

included long header and nav items in the example in OP too

@gabalafou
Copy link
Collaborator

Thanks for your feedback, I'll try to get back to you in the next few days

@sneakers-the-rat
Copy link
Author

re-read that and it comes across as more negative than i intended, i just meant i wasn't sure about y'alls process and if it's more of a pain than it's worth go ahead and do it however you'd normally do it, i don't have much time to fix it atm and just didn't want it hanging out indefinitely for y'all :). just trying to help is all not cause a headache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working tag: CSS CSS and SCSS related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants