Breadcrumbs #44 #213
Breadcrumbs #44 #213
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
jonatan-lledo-netcentric
changed the title
jonatan-lledo-netcentric
requested review from
SantiagoHomps-NC,
manuel-vara and
cogniSyb
There was a problem hiding this comment.
As per https://www.w3.org/WAI/ARIA/apg/patterns/breadcrumb/examples/breadcrumb/ I was expecting <nav> with <ol> and <li> to be used.
Clever solution, with the observers and calculations. Do I understand correctly that the demo with flexbox was not feasible for the requirements provided?
| outline: none; | ||
| } | ||
|
|
||
| .v2-breadcrumb__crumb:focus, |
There was a problem hiding this comment.
Remove the selector for :focus. We only provide the focus style when the user interacts with a keyboard, via :focus-visible
There was a problem hiding this comment.
done
| @@ -0,0 +1,55 @@ | |||
| .v2-breadcrumb-wrapper{ | |||
| background-color: var(--c-secondary-graphite); | |||
There was a problem hiding this comment.
I agree that we should use the variables. Though now that I checked, I see that #333 as color is defined for this
There was a problem hiding this comment.
I also checked it; this color doesn't yet have a proper variable. the other Figma file uses this color. updated
| outline-offset: 2px | ||
| } | ||
|
|
||
| .v2-breadcrumb__crumb:not(.v2-breadcrumb__crumb--home)::before { |
There was a problem hiding this comment.
I’d rather see ::before as part of an <li> so that it isn’t clickable.
The selector could be .v2-breadcrumb__crumb + .v2-breadcrumb__crumb::before, which is easier to read and more maintainable.
There was a problem hiding this comment.
it works the same than the use of :not(), so done
| const sectionStatus = 'data-section-status'; | ||
| const homeText = { | ||
| home: 'home', | ||
| ellipsis: '...', |
There was a problem hiding this comment.
Rather use a true ellipsis, which is one character. See https://practicaltypography.com/ellipses.html
There was a problem hiding this comment.
I didn't know about a single character for this. done
| block.textContent = ''; | ||
| block.append(...crumbs); | ||
| block.parentElement.classList.add('full-width'); | ||
| block.setAttribute('aria-label', 'Breadcrumb'); |
There was a problem hiding this comment.
Use getTextLabel, this content should be configurable in case of other languages
There was a problem hiding this comment.
placeholder updated. done
| export default function decorate(block) { | ||
| const cfg = readBlockConfig(block); | ||
| const hasPath = cfg && Object.hasOwn(cfg, 'path'); | ||
| const url = new URL(window.location.href); |
There was a problem hiding this comment.
As we’re not manipulating window.location, but rather reading contents of it, wouldn’t the following be easier?
let hasPath = cfg && Object.hasOwn(cfg, 'path');
let url = hasPath ? cfg.path : window.location.pathname;There was a problem hiding this comment.
is shorter and I like it, so done
| ellipsis: '...', | ||
| }; | ||
|
|
||
| const removePathDash = (str) => str.replace(/-/g, ' ').toLowerCase(); |
There was a problem hiding this comment.
Here you’re doing more than removing the dash from the path. You’re also transforming text to lowercase. So, could we rename it to formatText?
There was a problem hiding this comment.
agree, done
| return blockWidth - padding; | ||
| }; | ||
|
|
||
| const areCrumbsFit = (block) => getCrumbsWidth(block) < getBlockWidth(block); |
There was a problem hiding this comment.
areCrumbsFit confused me a bit. I think doCrumbsFit or even fitting would be clearer
There was a problem hiding this comment.
I like fitting, so done
| homeEl.textContent = homeText.home; | ||
| crumbs.forEach((crumb, i) => { | ||
| if (i === 0) return; | ||
| crumb.textContent = crumb.dataset.content; |
There was a problem hiding this comment.
These two lines are easier if it’s written like this:
if (index > 0) crumb.textContent = crumb.dataset.content;There was a problem hiding this comment.
done
| } | ||
|
|
||
| .v2-breadcrumb { | ||
| padding: 18px 16px 14px; |
There was a problem hiding this comment.
I think we should just do padding: 16px instead. At the 1200px breakpoint we should get rid of the horizontal padding, for alignment
There was a problem hiding this comment.
done
I added the nav and ul/li elements to follow the a11y pattern
the example removes all the elements from the end of the list and this doesn't work for our needs |
jonatan-lledo-netcentric
assigned girishdigrajkar and unassigned SantiagoHomps-NC and cogniSyb
New
the breadcrumb shows or hides based on the current width, even on resizing following the design pattern:
1st Home becomes an ellipsis
2nd current Active link is hided
3rd and beyond the middle links hide from the 2nd until the very last item
Test URLs
drafts/jlledo/v2/breadcrumb-demo
Fix #44
Test URLs: