-
Notifications
You must be signed in to change notification settings - Fork 3
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
SHS-6001: Accessibility issues in lists of people #1729
SHS-6001: Accessibility issues in lists of people #1729
Conversation
…ription to links inside card
…text inside card available to interact with
… the links inside structured card, except mailto and title links
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.
@mariannuar Looks good! Let's just organize the JS files a bit better:
- Rename
linked-cards
folder tocards
, movestructured-cards.js
there - Extract click and focus event handlers to a new file, add a wrapper function to assign all event handlers at once to cards
- Include the new file and call the wrapper function in both
linked-cards.js
andstructured-cards.js
This will help us find to have a better structure that goes along the JS restructuring done in #1691
@cienvaras Ready for review again! |
…-6001--fix-links-in-structured-card
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.
@mariannuar Looks great. There's just one small detail that we discussed in our call but forgot to add to the requested changes: let's use the is-focused
class that's being added when the title is focused to match the hover styles for the whole card (I think it's only the zoom effect on the image)
…title is focused to match the hover styles for the whole card
@cienvaras Done! |
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.
@mariannuar Works as expected, thanks for the fixes!
@ahughes3 Ready for you
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.
LGTM!
…structured card to unlink image
@cienvaras @ahughes3 Changes to unlink the image applied to Default People views |
@mariannuar @cienvaras @ahughes3 I merged this one but noticed Maria made a small change after the approvals. |
Thanks @joegl, yes, that's something extra we discussed during today's demo and I forgot to add a comment to let you know. It's a small config change, so shouldn't be a problem, but I'll take a look and confirm that everything works as expected. cc. @ahughes3 @mariannuar |
Awesome, thanks Andres 👍 |
Summary
Need Review By (Date)
01/30
Urgency
medium
Notes
Link media to parent
for theImage
field<a href="/"><span class="visually-hidden">Emilius Aalto: </span>Website</a>
, except mailto links and the title linkSteps to Test
/default-views/people
Default People view - People List Link (Grouped) Display
/default-views/news
<a href="/"><span class="visually-hidden">Emilius Aalto: </span>Website</a>
, except mailto links and the title linkReview Tasks
Backend / Functional Validation
Code
snake_case
and notcamelCase
?Code security
General