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

Replace unwrapped "up" and "down" strings used for aria labeling #12817

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

AllanOXDi
Copy link
Member

Summary

This PR audits and replaces unwrapped "up" and "down" strings used for aria labeling

References

#12392

Reviewer guidance.

review the slack conversation here for more context

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')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • 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: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Nov 11, 2024
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Hi @AllanOXDi - overall this is a solid start. Adding in the validation based on Richard's recommendation is great. I know we may still change some of the wording for the strings, but making sure that they are all translated strings, and making sure that all of the functions are returning strings properly, is important.

I know there are quite a few places where this draggable with up and down is used, but could you please run your local dev server, with the console open, and go to view each changed file, and resolve any console errors? I've added one example inline of a place where I can tell that there will be an issue, but I'd like you to go through each file, look at the console, and fix any problems. It shouldn't take long, but it's blocking. Thank you!

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Blocking:

  • Capitalize the message strings

Non-blocking:

  • I am conflicted about passing a Function that wraps a string. I love the idea of having a validation for passing along translated strings, but it is an unfamiliar approach.
  • I wonder if we should export your isWrappedString validator from the kolibri.utils.i18n library so that we could start using it elsewhere.
  • Namely this would be neat to somehow use with KDS as we pass many strings to those components.

Overall though this looks great - I think fixing Marcella's change request and capitalization should make this ready to go.

@AllanOXDi AllanOXDi force-pushed the fix-untranslated-prop-string branch 2 times, most recently from 58ec872 to 007de86 Compare November 18, 2024 10:38
@AllanOXDi AllanOXDi force-pushed the fix-untranslated-prop-string branch 2 times, most recently from eceef01 to c04562d Compare November 20, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) DEV: frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants