-
Notifications
You must be signed in to change notification settings - Fork 116
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
Set margin/padding properly on mobile breadcrumbs #8733
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! This looks good in TCCP and the handful of other pages I checked—never noticed the misalignment at tablet size before.
Hi! Thanks for tagging me @anselmbradford. And thanks so much for noticing and fixing the scrollbar issue @wpears and @niqjohnson. Yikesss!! I strongly support having consistent indentation for header/nav elements. I'm undecided regarding if we should make the indent 15px or 30px at tablet. I agree with what's been said regarding a 30px indent for header/nav elements making sense for aligning with the page's content. But I also see a case for a 15px indent in that it affords a little more horizontal space for breadcrumb text. Breadcrumbs can get long—especially in non-English languages—so anything we can do to help prevent breadcrumbs from wrapping to multiple lines I see as beneficial. I also like @anselmbradford thoughts regarding simplifying our indent approach to just desktop and tablet/mobile. So just to round things up, if we were to align elements at tablet size, we'd need to tweak the following. If we do 30px indentation, we'll need to change:
If we do 15px indentation, we'll need to change:
So, even split. :) Is one approach easier than the other dev-wise? |
@anselmbradford noted in #8732 that the underlying issue was probably in the breadcrumbs behavior itself and so I went looking to see why the breadcrumbs were causing the scrollbar in mobile.
Turns out, we use a negative margin + padding hack to bleed the background into the gutters at mobile/tablet. This was incorrectly set to 30px at BOTH tablet and mobile sizes, which caused the calculated width of the breadcrumbs container to be 30px larger than the browser width. I halved this at mobile which fixes the issue everything. That is, this breadcrumbs bug was happening in multiple places, not just tccp!
I also fixed what I believe is a bug in the tablet view, where the breadcrumbs aren't aligned with content (15px gutter instead of 30px). It's aligned in both mobile and desktop, so I figured we should align it at tablet too.
This supersedes #8732, so I stripped out the special-casing there.
Testing