yari
yari copied to clipboard
mdn
enhance(breadcrumbs): change breadcrumbs behaviour and style in small sizes for better UX
Summary
Just a proposal from a user frequently using the app on mobile. I found it better to show the previous nav item instead of the root for ex: " Array> Array.prototype.slice() " instead of "References > Array.prototype.slice()"
Also, it handles breadcrumbs wrapped layout.
In addition to that, it shows the last two items only in small sizes.
Fixes #10385, #8250, #8627
Problem
Navbar shows 2 elements in small sizes ("root element", and "current page") and I just found the "root element" not needed by the user unlike the "parent of the current page"
Navbar not handling wrapped layouts.
Navbar shows 2 elements on a wide screen sizes where other items can be shown
Solution
I suggest to replace the "root element" with the "parent of the current page" so that we can provide the users with better experience in my opinion
handle wrapped layouts by adding spaces between breadcrumbs items
show 2 elements only in small sizes
Screenshots
Before
1- root and last items
2- wrapped layout with no space
3- show 2 items even in wide screens
After
1-last two items
2-wrapped layout with space
3-show 2 items in small screens only (size < 426px)
@Ahmed-Hakeem Thanks for opening this PR, but please make sure to follow and fill out the PR description template:
https://github.com/mdn/yari/blob/9838d6ca60bd72e0c32cff9f5ffa3546de3fc3d4/.github/PULL_REQUEST_TEMPLATE.md#L1-L14
@Ahmed-Hakeem Thanks for opening this PR, but please make sure to follow and fill out the PR description template:
https://github.com/mdn/yari/blob/9838d6ca60bd72e0c32cff9f5ffa3546de3fc3d4/.github/PULL_REQUEST_TEMPLATE.md#L1-L14
sure thing
Fixes https://github.com/mdn/yari/issues/8627 raised by @wbamberg by showing the parent instead of the root
This is better but it doesn't fix the issue. In my screenshot there's plenty of space to show all levels but Yari only shows one ancestor. In this PR, IIUC, in my example page we would see:
Guides > Making PWAs installable
...meaning I can't use the breadcrumbs to go to the top of the PWA docs (or use the breadcrumbs to see the structure).
Fixes #8627 raised by @wbamberg by showing the parent instead of the root
This is better but it doesn't fix the issue. In my screenshot there's plenty of space to show all levels but Yari only shows one ancestor. In this PR, IIUC, in my example page we would see:
Guides > Making PWAs installable...meaning I can't use the breadcrumbs to go to the top of the PWA docs (or use the breadcrumbs to see the structure).
Yeah, I got it; I was about to mention that. It's not completely solving it.
I just didn't want to raise another issue with part of your concern. I found you saying, "At least that will enable me to get to the parent page," so that's why.
Yes, totally fair - the only reason I'm being pedantic here is that adding "Fixes
Yes, totally fair - the only reason I'm being pedantic here is that adding "Fixes " in the description means the bug will be auto-closed if/when the PR is merged, which IMO would not be appropriate.
I have updated the description to clarify that
Overall LGTM from a code perspective, just one nit regarding px vs rem, and just FYI this change needs approval from our product team, but I'll ping them.
that's fine, waiting for it
Thank you
Thank you @caugner for making changes on this one. I just want to mention that issue #8250 still solved with the new changes, also why we revert the breakpoint to be xl since this was what actually solves #8627 and has no side effect
I just want to mention that issue #8250 still solved with the new changes,
Thank you, good catch, I have closed the issue again now. 👍
also why we revert the breakpoint to be xl since this was what actually solves #8627 and has no side effect
Good question. We need approval from our UX designer for UI changes like this one, but we're working on a redesign this year, so it doesn't make sense to change details like this, especially as it had unintended side-effects (note the locale line break and the heading - navigated to via the TOC - being slightly hidden by the header):
Surely these side-effects can be fixed/mitigated, but we don't want to invest time into this right now, so we're not accepting PRs to fully resolve #8627 at this time.
I just want to mention that issue #8250 still solved with the new changes,
Thank you, good catch, I have closed the issue again now. 👍
also why we revert the breakpoint to be xl since this was what actually solves #8627 and has no side effect
Good question. We need approval from our UX designer for UI changes like this one, but we're working on a redesign this year, so it doesn't make sense to change details like this, especially as it had unintended side-effects (note the locale line break and the heading - navigated to via the TOC - being slightly hidden by the header):
Surely these side-effects can be fixed/mitigated, but we don't want to invest time into this right now, so we're not accepting PRs to fully resolve #8627 at this time.
Thank you for your clarification : ) …. excited to see the new design ❤️. Keep up the good work.