-
Notifications
You must be signed in to change notification settings - Fork 2
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
Peek-a-boo header strip #1317
base: main
Are you sure you want to change the base?
Peek-a-boo header strip #1317
Conversation
Code looks fine. The design is a bit clunky, but it always was going to be, switching between the proper header and the peekaboo header. I like that you didn't have to duplicate the content for the peekaboo nav. The one question I have is about on small screens — I see that the buttons go away and are replaced with the menu button, just like the proper header. Will the menu still be positioned properly if you're scrolled away from the top when it opens? I assume it'll work, since I'm guessing it's positioned relative to the header, not the page, but I just wanted to confirm that. That's not a blocker, though, since this is just the pattern library representation, so I guess I'm just making a mental note to check that when we implement it on the actual site. I'll defer on design approval to @heyitsal, but approving now for code review. |
I'm definitely a small screen person who would be happy to give this a look-see when it's on the live site. |
You can play with it here: https://deploy-preview-1317--mltshp-patterns.netlify.app/?path=/story/legacy-components-site-header--logged-in-with-content |
Ah I forgot to check light mode. I can address that easily enough. The slide in/out should also occur once scrolling starts not once it ends; I'll address that as well. Finally, I think we could do something with the mobile version since there is a lot of empty space left after condensing the logo to an icon. Any thoughts on that? |
…ns into feature/dynamic-nav
Looks like it just works (manually applied the Also updated the background for light mode, and removed the debounced function so it shows/hides the nav strip more promptly on scroll. |
Special consideration may be needed for the expanded menus... if that menu is very long and requires scrolling, scrolling downward may cause the header and menus to disappear as it is (although, the dismiss on scroll does not occur for gradual scrolling... you have to scroll > 20px since last scroll event to trigger it). |
Should be kept within the viewport, scrolling contents when it overflows.
Updated the PR to restrict maximum height of the shake list to |
Can you elaborate with specific details? Anything I can improve on? |
Honestly, upon further reflection, I think it was mostly that the dark background looked weird in light mode. I clicked through to the Storybook preview deploy to review your latest changes, but the peekaboo isn't working? I'm using Edge. |
Working for me in Edge (latest). Any console errors? |
After testing again, I now see the peekaboo header with the same cut-off bug you described above. Sorry! |
Say, regarding the cut-off thing... I sometimes see this happen on image content on the site. I've only ever seen it happen on Chromium-based browsers (I'm using Arc these days, but pretty sure it's happened in Chrome too). Any idea what could be causing it? It's an odd rendering glitch. If I drag to select the image causing it to repaint, it does. |
Overview
Implementation of 'peek-a-boo' site header.
Screenshots
Testing
Available in Storybook under Legacy -> Components -> Site Header -> Logged in With Content
Scroll down until header area is off-screen, then scroll up to reveal the header. Scrolling downward will hide it again. There is no non-interaction timer for automatic hiding after reveal.