Adding role menu and menuitem to navigation menu trees and leafs #9149
Conversation
| @@ -1,6 +1,6 @@ | |||
| {% macro submenuItem(name, url, anchorlink) %} | |||
| {% if name|slice(0,1) != '_' %} | |||
| <li> | |||
| <li role="menuitem" title="{{ name|translate }}"> | |||
There was a problem hiding this comment.
add |e('html_attr') for proper attribute escaping
|
Thank you for support on twig filters. I'm going to update my PR asap 👍 |
| <a class="item" href="{% if anchorlink %}#{% else %}index.php?{% endif %}{{ url|urlRewriteWithParameters|slice(1) }}"> | ||
| {{ name|translate }} | ||
| {{ name|translate|e('html_attr') }} |
There was a problem hiding this comment.
There should be no |e('html_attr) here. It should be only used when it will be used within an HTML attribute like above in the title attribute.
|
Note to myself: We need to make sure to apply this in Piwik 3.0 branch as well |
| {{ 'CoreHome_Menu'|translate }} | ||
| </span> | ||
| <span class="menu-icon {{ level2._icon|default('icon-arrow-right') }}"></span> | ||
| {{ level1|translate }} |
There was a problem hiding this comment.
Sorry to bother you again, I noticed some UI tests are failing now because the {{ level1|... } is no longer directly attached to the </span> as it was the case before.
This results in a whitespace between the icon and the menu name and so the menu item names are no longer aligned
BTW: Are you familiar with squashing commits?
There was a problem hiding this comment.
No problem for bothering, I'm aware of my situation, learning by mistakes 🎓
Thank you for the link about squashing commits, I was unaware because not mentioned in Docs. (I've added a PR also on it)
Adding role menu and menuitem to navigation menu trees and leafs
|
Thx a lot for this @tassoman well done :) |
tsteur
added
the
not-in-changelog
|
Thank you I'm proud I can contribute to Piwik's improvements! |
|
Thanks for your help @tassoman . We feel very lucky to get such contributions. This will soon run on hundred thousands of servers and will be used by many users :) Feel free to issue another PR any time. Cheers |
mattab
added
the
c: Accessibility
This pull request is a first solution to #9148 .
For better results in terms of a11y, I think would be better removing a href="" links from trees menu items (as seen in this Success criteria example code) then managing hide/show behavior by using aria-hidden="true" attribute or jquery code. This part is not covered in this pull request.
If you like the solution I could go on further with another pull request.