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.

Sign up for GitHub

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

Jump to bottom

STCOR-753: Add aria-label for ProfileDropdown.js #1382

Merged
merged 7 commits into from
Jan 17, 2024
Merged

STCOR-753: Add aria-label for ProfileDropdown.js #1382

merged 7 commits into from
Jan 17, 2024

Conversation

UladzislauKutarkin
Copy link
Contributor

@UladzislauKutarkin UladzislauKutarkin commented Dec 14, 2023 •
edited by zburke
Loading

STCOR-753
Here I reproduced steps that was provided by @JohnC-80 in comments section.
We add aria-label for ProfileDropdown. Also, we removed aria-label from App dropdown.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 14, 2023 •
edited
Loading

BigTest Unit Test Statistics

    1 files  ±0      1 suites  ±0   10s ⏱️ ±0s
271 tests ±0  265 ✔️ ±0  6 💤 ±0  0 ❌ ±0 
274 runs  ±0  268 ✔️ ±0  6 💤 ±0  0 ❌ ±0 

Results for commit 3e5bcca. ± Comparison against base commit 01c5d6c.

This pull request removes 5 and adds 3 tests. Note that renamed tests count towards both. 
      equal to check email label in english translation
      equal to check email precautions label in english translation
      equal to sent email precautions label in english translation
Chrome_120_0_0_0_(Linux_x86_64).Forgot username form test check email status page tests ‑ Forgot username form test check email status page tests should have the header with an appropriate text content
Chrome_120_0_0_0_(Linux_x86_64).Forgot username form test check email status page tests ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content

Chrome_120_0_0_0_(Linux_x86_64).Forgot username form test check email status page tests ‑ Forgot username form test check email status page tests should have the header with an appropriate text content
      equal to check email label in english translation
Chrome_120_0_0_0_(Linux_x86_64).Forgot username form test check email status page tests ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content
      equal to check email precautions label in english translation
Chrome_120_0_0_0_(Linux_x86_64).Forgot username form test check email status page tests ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content
      equal to sent email precautions label in english translation

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 14, 2023 •
edited
Loading

Jest Unit Test Statistics

137 tests  ±0   137 ✔️ ±0   23s ⏱️ -1s
  18 suites ±0       0 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 3e5bcca. ± Comparison against base commit 01c5d6c.

♻️ This comment has been updated with latest results.

@UladzislauKutarkin UladzislauKutarkin requested review from zburke and a team December 26, 2023 09:39
@Dmitriy-Litvinenko Dmitriy-Litvinenko requested a review from a team December 26, 2023 14:18
zburke
zburke requested changes Jan 2, 2024
View reviewed changes
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

As @Dmitriy-Litvinenko noted, the new aria-label must be handle via react-intl so it can be localized.

@UladzislauKutarkin
Copy link
Contributor Author

As @Dmitriy-Litvinenko noted, the new aria-label must be handle via react-intl so it can be localized.

Done!

@Dmitriy-Litvinenko Dmitriy-Litvinenko requested a review from a team January 4, 2024 11:00
zburke
zburke requested changes Jan 4, 2024
View reviewed changes
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

Close, but not quite right. formatMessage() requires two arguments.

@zburke zburke changed the title STCOR-753: Add arial-label for ProfileDropdown.js STCOR-753: Add aria-label for ProfileDropdown.js Jan 8, 2024
@NikitaSedyx NikitaSedyx requested a review from zburke January 9, 2024 05:43
zburke
zburke requested changes Jan 10, 2024
View reviewed changes
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

Sorry, still not quite right, and sorry I didn't catch this sooner: closely reviewing @JohnC-80's request on STCOR-753, this PR must "expand the accessible label" of the profile dropdown; instead, it adds a new label.

Instead of adding an aria-label in renderProfileTriggerLabel (line 272), you need to update renderProfileTrigger and provide a correct ariaLabel to the <NavButton> there (line 248).

@UladzislauKutarkin
Copy link
Contributor Author

Sorry, still not quite right, and sorry I didn't catch this sooner: closely reviewing @JohnC-80's request on STCOR-753, this PR must "expand the accessible label" of the profile dropdown; instead, it adds a new label.

Instead of adding an aria-label in renderProfileTriggerLabel (line 272), you need to update renderProfileTrigger and provide a correct ariaLabel to the <NavButton> there (line 248).

@zburke please take a look. I made requested changes

zburke
zburke requested changes Jan 11, 2024
View reviewed changes
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

@UladzislauKutarkin, is there a reason you created a new translation key mainnav.profile.ariaLabel instead of repurposing the existing one, mainnav.myProfileAriaLabel?

Either reuse the existing one (preferable) or keep the new one and remove the old one.

@UladzislauKutarkin
Copy link
Contributor Author

UladzislauKutarkin commented Jan 12, 2024 •
edited
Loading

@UladzislauKutarkin, is there a reason you created a new translation key mainnav.profile.ariaLabel instead of repurposing the existing one, mainnav.myProfileAriaLabel?

Either reuse the existing one (preferable) or keep the new one and remove the old one.

@zburke Ye, you are right, I made new one key for prev implementation. Since we made new approach, I returned old translation key and deleted new one

zburke
zburke approved these changes Jan 16, 2024
View reviewed changes
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

LGTM! 👏 👏 👏

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
80.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@UladzislauKutarkin UladzislauKutarkin merged commit 35799e8 into master Jan 17, 2024
6 checks passed
@UladzislauKutarkin UladzislauKutarkin deleted the STCOR-753 branch January 17, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zburke zburke zburke approved these changes

@Dmitriy-Litvinenko Dmitriy-Litvinenko Dmitriy-Litvinenko approved these changes

@Terala-Priyanka Terala-Priyanka Terala-Priyanka approved these changes

@JohnC-80 JohnC-80 Awaiting requested review from JohnC-80

@vashjs vashjs Awaiting requested review from vashjs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@UladzislauKutarkin @zburke @Dmitriy-Litvinenko @Terala-Priyanka