-
Notifications
You must be signed in to change notification settings - Fork 916
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
Add breadcrumbs to taxonomy result list #1011
Add breadcrumbs to taxonomy result list #1011
Conversation
This is great! @narrenfrei if you're free do you want to take a closer look at this as you did a lot of the taxonomy support? I'll take a look as well. |
Looks fine - thank you for the great work! |
Looks good to me! Thanks again. |
Follow-up: can you add a quick update to the docs telling users how to disable the taxonomy breadcrumb feature if required? |
@@ -18,8 +18,11 @@ | |||
{{ template "breadcrumbnav" (dict "p1" .p1.Site.Home "p2" .p2 ) -}} | |||
{{ end -}} | |||
{{ $isActive := eq .p1 .p2 }} | |||
<li class="breadcrumb-item{{ if $isActive }} active{{ end }}" | |||
{{- if $isActive }} aria-current="page"{{ end }}> |
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.
@at055612 - why did you remove aria-current
?
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.
Good question. I can't remember why I took that out. Maybe because I don't think it is applicable when the breadcrumb is shown on the taxonomy list, however I agree it ought to be there for the page breadcrumbs. Will take a look.
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.
For now I've reverted this change -- see #1068. We would need a bit more logic to remove (all?) the ARIA related labels for the taxonomy-list usage ... or maybe just use a regex to strip all aria labels from breadcrumbs in taxonomy list. I'll let you follow up on that if you think that it's worth it.
* googlegh-1004 Add breadcrumbs and cards to taxonomy result list * googlegh-1004 Change breadcrumb partial so leaf item is not a link * googlegh-1004 Format scss Co-authored-by: LisaFC <lcarey@google.com>
Fixes #1004
taxonomy_breadcrumb_disable
, enabled by defaultI think the cards could do with some drop shadow but I haven't added it as I wasn't sure it was consistent with the rest of the theme.
Making the leaf item not a link is possibly contentious but a quick google suggests this is an accepted practice. I think having a link to the page you are on is confusing and offers no value.
This is how it now looks on the example site
This is how it looks for a result that is at the root level.
But if
.td-breadcrumbs__single { display: none !important; }
is set in the project scss then you will see:The page breadcrumbs now look like this: