-
Notifications
You must be signed in to change notification settings - Fork 25
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
[WIP] fix(esco-content-menu): accessibility improvements #440
[WIP] fix(esco-content-menu): accessibility improvements #440
Conversation
@jonathanmtran your change on PortletCards about the href part are welcome but this will broke the content-favorites where this is used, so please try to repeat this change on the whole view of the component, ie the esco-hamburger-menu or at least the content-menu. Also did you test the builded component from the demo page as example ? |
I should have tagged this as work in progress as the changes were done in order for us to be allowed to deploy |
@jonathanmtran could you refresh your WIP ? I have a chance to make a review on this work. |
Also for your development please test the deployment on an uPortal instance, some parts won't works like in the Thanks |
da1fcc1
to
a391416
Compare
@jgribonvald My branch has been rebased against master |
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.
Some change are required as it seems that some things are missing, also the change applied on content-grid + portletCard weren't applyed on favoritesContent. A change on this part is required if you want to change the href position around portletCard.
Also did you make a test on builded sources within an uPortal instance ?
@@ -46,31 +46,24 @@ | |||
</div> | |||
</slot> | |||
</div> | |||
<div class="status sr-only" aria-live="polite">{{ statusMsg }}</div> |
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.
To add the sr-only class you need to import the css common part as after building the sr-only class isn't provided !
@import './../styles/common.scss';
should be added to style part
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 can't find any information change about the status, is it shown only after clicking on add/remove the favorite link ?
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.
To add the sr-only class you need to import the css common part as after building the sr-only class isn't provided !
@import './../styles/common.scss';
should be added to style part
Will do. Thanks
I can't find any information change about the status, is it shown only after cliquing on add/remove the favorite link ?
It isn't shown at all. The goal of this change is to provide feedback to a user using a screen reader that a portlet has been favorited/unfavorited
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 can't find any information change about the status, is it shown only after cliquing on add/remove the favorite link ?
It isn't shown at all. The goal of this change is to provide feedback to a user using a screen reader that a portlet has been favorited/unfavorited
OK so for accessibility changing the icon and message to favorited from unfavorited state (and the reverse action) isn't enough !
@@ -5,8 +5,10 @@ | |||
"filter": "Find a service..." | |||
}, | |||
"favorites": { | |||
"add": "Add to favorites", | |||
"remove": "Remove from favorites", | |||
"add": "Add \"{}\" to favorites", |
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 feel like something is missing as I can't see change that pass a param for the translation.
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.
Also could you provide the same change on French files, and a French people will fix it if something is needed don't be worried !
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 feel like something is missing as I can't see change that pass a param for the translation.
In favoriteMessage()
in ActionFavorites.vue
and in actionToggleFav()
in ContentGrid.vue
, I'm doing a string replacement to replace the {}
placeholder with the name of the portlet. If there's a better way to accomplish this, please advise.
Also could you provide the same change on French files, and a French people will fix it if something is needed don't be worried !
I updated the French language files. Hope it's a good starting point
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.
Thanks 🥇
Yes. I've tested the resulting Webjar against a clean copy of uPortal-start. As far as I'm aware, the admin tools tab/layout is using the |
Firstly you can test the whole part from the project in running |
@jonathanmtran friendly ping, is this still in progress? |
I haven't had the time to remediate the other components in this family of components, so I'll be closing this |
We've made various tweaks to the content grid to improve accessibility
a
elementsupersedes PR #428
resolves issue #431
resolves issue #438