Skip to content
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

Upgrade KDS to v5.0.0-rc1, introduce KDS live region and use it at one place #12475

Merged
merged 5 commits into from
Aug 8, 2024

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Jul 23, 2024

Summary

References

Reviewer guidance

Aug 7 update: below won't work as I cleaned up temporary changes that allowed for testing.

Code review

  • Skip reviewing 6a573c8, it's just nonsense changes to allow for testing and will be removed

QA

  • As a learner, navigate to Learn > Home. You should see the banner with "You do not have enough storage for new resources. Ask your coach or administrator for help." message right away. Hear it being announced by the screen reader after all other initial announcements, such as Kolibri was loaded.
    • Note that the banner will be displayed under any circumstances, even wrong ones - just for the purpose of testing
    • Note that the screen reader will also announce the number of points if you have any in a way that is rather confusing, just saying 500 without any context (at least my Orca does this) - not in the scope of this PR.
  • After 15 seconds, the banner message changes to "Updated storage notification banner message that should be announced by screen readers." This too should be announced.
  • Also observe browser console, particularly see if you spot any [useKLiveRegion] Could not send the message: ... error

Note that it's better to use screen reader to test this, rather than just observing

Screenshot from 2024-07-23 22-17-19

because the content of region's elements gets cleared up really fast (deliberate and recommended behavior), making it nearly impossible to spot it.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • IIf this is an important user-facing change, PR or related issue has a 'changelog' labelI
  • IIf this includes an internal dependency change, a link to the diff is providedI

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend labels Jul 23, 2024
@MisRob MisRob force-pushed the use-live-region branch 2 times, most recently from c12043a to 6a573c8 Compare July 23, 2024 19:13
@MisRob MisRob added the TODO: needs review Waiting for review label Jul 23, 2024
@MisRob MisRob marked this pull request as ready for review July 23, 2024 20:13
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes make sense to me :)

@radinamatic
Copy link
Member

radinamatic commented Aug 6, 2024

As per the reviewer guidance, NVDA output on Windows 10 was as expected:

Kolibri  busy
graphic    Kolibri loader

You do not have enough storage for new resources 

After 15 seconds, NVDA outputs the update:
Updated storage notification banner message that should be announced by screen readers.

(Outside of scope) NVDA also announces the points flat (when earned):

Kolibri  busy

You do not have enough storage for new resources. Ask your coach or administrator for help. 
Learn
500  
Updated storage notification banner message that should be announced by screen readers. 

We should investigate this further, maybe the quickest solution would be to remove the role="presentation" from the the leaf SVG because it invalidates its aria-label="Poins earned".

The documentation page at https://deploy-preview-687--kolibri-design-system.netlify.app/usekliveregion/ reads as a good foundation. I'm unaware of a more specific guidance around the use of polite vs assertive than what you already included: the latter is always recommended only for status messages that require an immediate attention by the user, like error messages.

The only thing that could potentially be also included in the guidance is the reminder that when the assertive status notification is used, and there is an actionable element like a button that goes with it, that button should be the next in the focus order after the notification is presented to the user.

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Live region performing as per specification, looking forward to seeing these implemented across Kolibri! 👍🏽 💯 :shipit:

@MisRob
Copy link
Member Author

MisRob commented Aug 7, 2024

Thanks @radinamatic!

The points issue logged here - #12326 (comment)
Docs update follow-up here - learningequality/kolibri-design-system#716

MisRob added 2 commits August 7, 2024 17:41
user haven't had enough storage from the very
beginning of starting Kolibri.
@MisRob MisRob changed the title [DO NOT MERGE UNTIL NEW KDS RELEASE] Introduce KDS live regions and use them at one place Upgrade KDS to v5.0.0-rc1, introduce KDS live region and use it at one place Aug 7, 2024
@MisRob MisRob requested a review from AlexVelezLl August 7, 2024 15:49
@MisRob
Copy link
Member Author

MisRob commented Aug 7, 2024

Cleaned up temporary commits and installed the latest KDS release. Ready for final review and merge. @AlexVelezLl would you give it one last look?

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Would be ready to go after removing the extra space of the version in the package.json 👐.

@@ -21,7 +21,7 @@
"js-cookie": "^3.0.5",
"knuth-shuffle-seeded": "^1.0.6",
"kolibri-constants": "0.2.6",
"kolibri-design-system": "4.3.2",
"kolibri-design-system": "5.0.0-rc1 ",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there is an extra space, I dont think this could cause a problem, but just in case.

Suggested change
"kolibri-design-system": "5.0.0-rc1 ",
"kolibri-design-system": "5.0.0-rc1",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes better to fix. Done

@MisRob
Copy link
Member Author

MisRob commented Aug 8, 2024

Check failure not related, resolved in #12549

@MisRob MisRob merged commit 9dcb652 into learningequality:develop Aug 8, 2024
32 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants