fix: fix: use aria attribute correctly resolves wg-translations/issue… #1162
Conversation
…s/33 This change correct the usage of aria-* attributes
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ghassanmas! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1162 +/- ##
=======================================
Coverage 92.68% 92.68%
=======================================
Files 693 693
Lines 12346 12346
Branches 2699 2699
=======================================
Hits 11443 11443
Misses 872 872
Partials 31 31 ☔ View full report in Codecov by Sentry. |
| aria-labelledby={intl.formatMessage(messages.alertSuccessAriaLabelledby)} | ||
| aria-describedby={intl.formatMessage(messages.alertSuccessAriaDescribedby)} | ||
| aria-label={intl.formatMessage(messages.alertSuccess)} | ||
| aria-description={intl.formatMessage(messages.alertSuccessDescriptions)} |
There was a problem hiding this comment.
I think we still want to have aria-labelledby here, but just use the element id directly instead of using intl.formatMessage for it
There was a problem hiding this comment.
Yes it's one option, the thing is that two ids would need to pass from parent down to Alert componenet, I have an example here 3c5541a , if you think it's okay then I guess we do the same for all other usage?
ghassanmas
force-pushed
the
resolve-twg-33
branch
from
f56f44b to
3c5541a
Compare
|
Wow, thanks for cleaning this up @ghassanmas :) |
| @@ -244,8 +242,8 @@ const AdvancedSettings = ({ intl, courseId }) => { | |||
| <AlertMessage | |||
| show={saveSettingsPrompt} | |||
| aria-hidden={saveSettingsPrompt} | |||
| aria-labelledby={intl.formatMessage(messages.alertWarningAriaLabelledby)} | |||
| aria-describedby={intl.formatMessage(messages.alertWarningAriaDescribedby)} | |||
| aria-labelledby="advancedSettingsAlertWarningTitle" | |||
There was a problem hiding this comment.
| aria-labelledby="advancedSettingsAlertWarningTitle" | |
| aria-labelledby="notification-warning-title" |
There was a problem hiding this comment.
we used have
alertWarningAriaLabelledby: {
id: 'course-authoring.advanced-settings.alert.warning.aria.labelledby',
defaultMessage: 'notification-warning-title',
},and then we used that message in
aria-labelledby={intl.formatMessage(messages.alertWarningAriaLabelledby)}which means this evaluates to
aria-labelledby="notification-warning-title"
|
Hi @ghassanmas! Just checking to see if you're still planning to pursue this pull request? |
…s/33
This change correct the usage of aria-* attributes resolves
aria-describedbyandaria-labelledbyuntranslatable as they shouldn't be wg-translations#33Description
Before in part of code,
aria-describedbyandaria-labelledbyattributes were treated as having translatable text, which is not correct, since these attributes suppose to link/refer to ids.This PR resolve the issue by using
aria-label, andaria-descriptioninstead, which can include raw text.Note: the eslint role for aria is switched off since,
aria-descriptionis a new attribute that is not recgnoized until react>=18 hence this issue in react.Lastly, aria-descripedby and aria-labelledby could have been used, but that would have require some extensive code change, so that
idis passed on the childern ofAlretparagon component.This ready for review