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

ENH: Move header items to sidebar on mobile #834

Merged
merged 12 commits into from Jul 28, 2022

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Jul 22, 2022

This is the implementation that I used to show the preview in #833 - it is actually pretty close to complete if people think that this is a good UI/UX for the navbar / sidebar content. It does this:

  • Duplicates our center and end header items in the sidebar.
  • Wraps those items in the sidebar only show up on mobile screens.
  • Makes those items in the header only show up on wide screens.
  • Moves over the @media queries so that they apply only to the sidebar items.
  • Generalizes the navbar link styling to a component rather than being hard-coded in the section scss.
  • Makes us use class names that are BEM-style, rather than IDs, for our theme and version switcher so we can have multiple
  • Cleans up some duplicated CSS that was related to navigation links and sidebars
  • Factors out the JavaScript that was embedded in our version switcher, and now uses a combination of global variables and puts the main JS into our index.js script.
  • Standardizes some more CSS variables so that some items (like icons) behave the same in mobile/widescreen

Here's how it looks

chrome_D1348DGCxG

To Do

  • Agree this is a good UI/UX to use
  • Wait for ENH: Search button shows up on mobile #832 and incorporate here
  • Fix any remaining CSS bugs
    • "I am just seeing some spacing inconsistencies around the icons (GitHub, Twitter, etc.)."
    • Fix version switcher populating JS
  • Double-check that the JS and CSS still work

Let me know what folks think.

Release notes

The header items are now duplicated on each page, and wrapped in <div>s that will alternate displaying at the sidebar-primary breakpoint. The effect is that the header items after the "start" block are inside the sidebar on mobile.

❗This changes the HTML of the theme and version switchers slightly!

@drammock
Copy link
Collaborator

beautiful. +10

@tupui
Copy link
Contributor

tupui commented Jul 22, 2022

Nice 👍 it's really a better experience on mobile. I am just seeing some spacing inconsistencies around the icons (GitHub, Twitter, etc.).

@choldgraf
Copy link
Collaborator Author

I will clean up the spacing inconsistencies and little CSS bugs if I know people like the general UI/UX :-)

@12rambau
Copy link
Collaborator

I love the idea !

@choldgraf
Copy link
Collaborator Author

Update: ready for review

OK I think this is one is ready for review. I've cleaned up the remaining CSS bugs that I found and included a bit more standardization of our variables etc. Let me know what you think.

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

nice job figuring out how to move the version switcher JS out of the template! This all looks good to me, though in the process of reviewing I found a few (unrelated) things to open issues about :)

Copy link
Contributor

@tupui tupui left a comment

Choose a reason for hiding this comment

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

This looks great 👍

About the version switcher, it's a breaking change on the styling correct?
We need to go from #version_switcher to .version-switcher__container? I am fine with this since we always update to the latest version of the theme (at least I do this for SciPy and NumPy).

@choldgraf
Copy link
Collaborator Author

indeed this would be a breaking change for any custom styling on the version switcher...though in general it feels like this is also a better practice to move to BEM-style classes instead of ids

@jarrodmillman jarrodmillman added this to the 0.10 milestone Jul 26, 2022
@choldgraf
Copy link
Collaborator Author

choldgraf commented Jul 28, 2022

Hey all - we've got 3 approvals, so I am going to merge this one in! Many thanks for everybody's feedback, I think this is a nice place to settle on for the sidebar UX!

@choldgraf choldgraf merged commit 703318d into pydata:main Jul 28, 2022
@choldgraf choldgraf deleted the header-in-sidebar branch July 28, 2022 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants