-
Notifications
You must be signed in to change notification settings - Fork 615
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
breadcrumbs last element #1403
breadcrumbs last element #1403
Conversation
Autotagging @bigcommerce/storefront-team @davidchin |
@bc-zoharmuzafi Doesn't that just duplicate the breadcrumb navigation entry. Why even have it as an item if there is no URL for it? |
Googles docs on Breadcrumbs is not very clear on if you should include the current page in the breadcrumb: https://developers.google.com/search/docs/data-types/breadcrumb I personally don't include it, and it makes no difference to the rich snippets produced. Google does not include the current page as part of the trail. Google does specify that the position should start with 1, not zero. It maybe worth fixing that, but it causes no harm. Their example also does not include the home page. Again, I've found that including it or not makes no difference to the breadcrumbs. Google does not include the root as part of the trail. I include it for BC sites, just because the code is simpler. On a related note, if say the product url is added as the id for the last item, then that often clashes with the id used for the Product entity on the same page. This can cause the product entity to be merged into the breadcrumb entity. One common method to avoid this id clashing is to add a # value to the end of the ids. e.g. "@id": "https://.../product-page#Breadcrumb" and "@id": "https://.../product-page#Product" Then multiple entities can be on the same page and still have their own id based on the URI for the page. |
{{#if url}} | ||
<a href="{{url}}" class="breadcrumb-label" itemprop="item"><span itemprop="name">{{name}}</span></a> | ||
{{else}} | ||
{{#if @last}} |
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.
I would change this to
if (last || url.empty) {
don't create an anchor tag
} else {
create an anchor tag
}
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.
@mattolson fix that condition
What?
Fixed the position for breadcrumb elements to start with 1. instead of 0.
Screenshots (if appropriate)
Before
After