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

[#12081] User-friendliness: Fix feedback path dropdown #12207

Merged

Conversation

weiquu
Copy link
Contributor

@weiquu weiquu commented Mar 15, 2023

Part of #12081
Sub-issue: Fix feedback path dropdown

Outline of Solution
Changed second dropdown to be below instead of to the side. Keeps track of which submenus are open via a map. Also added buttons for accessibility and added the text-wrap class to resolve overflow issues.

For E2E tests, changed the options to click on the button.

@weiquu
Copy link
Contributor Author

weiquu commented Mar 15, 2023

I didn't put aria-labels on the feedback path dropdown options since the screen reader already picks up the text inside each button. I did, however, put one on the overall feedback path dropdown button. The issue is that the aria-label is currently set to just "Feedback Path Dropdown", and does not indicate what feedback path is currently selected.

Given this situation, should I remove the aria-label so that the screen reader just mentions what is selected + that it is a button, or should I edit the aria-label to also take into account the currently selected path?

@zhaojj2209
Copy link
Contributor

Given this situation, should I remove the aria-label so that the screen reader just mentions what is selected + that it is a button, or should I edit the aria-label to also take into account the currently selected path?

I think in this case it would be alright to remove the aria label.

Also, do fix the E2E tests, this PR seems to have broken a few.

@zhaojj2209 zhaojj2209 added the s.ToReview The PR is waiting for review(s) label Mar 15, 2023
@samuelfangjw
Copy link
Member

Snyk is currently down which explains the failing test.

@weiquu
Copy link
Contributor Author

weiquu commented Mar 15, 2023

I think in this case it would be alright to remove the aria label.

Also, do fix the E2E tests, this PR seems to have broken a few.

Thanks, updated!

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

@zhaojj2209 zhaojj2209 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Mar 16, 2023
@zhaojj2209 zhaojj2209 merged commit 8cfd899 into TEAMMATES:user-friendliness Mar 16, 2023
zhaojj2209 pushed a commit that referenced this pull request Mar 23, 2023
* Shift submenu to bottom

* Clean up code

* Change to buttons for accessibility and update chevron

* Fix screenreader focus

* Lint and test

* Fix mobile overflow

* Fix lint issues

* Remove aria-label and update tests

* Fix attempt for E2E tests

* Update tests

* Refine E2E change
zhaojj2209 pushed a commit that referenced this pull request Mar 23, 2023
* Shift submenu to bottom

* Clean up code

* Change to buttons for accessibility and update chevron

* Fix screenreader focus

* Lint and test

* Fix mobile overflow

* Fix lint issues

* Remove aria-label and update tests

* Fix attempt for E2E tests

* Update tests

* Refine E2E change
zhaojj2209 pushed a commit that referenced this pull request Mar 25, 2023
* Shift submenu to bottom

* Clean up code

* Change to buttons for accessibility and update chevron

* Fix screenreader focus

* Lint and test

* Fix mobile overflow

* Fix lint issues

* Remove aria-label and update tests

* Fix attempt for E2E tests

* Update tests

* Refine E2E change
zhaojj2209 pushed a commit that referenced this pull request Mar 25, 2023
* Shift submenu to bottom

* Clean up code

* Change to buttons for accessibility and update chevron

* Fix screenreader focus

* Lint and test

* Fix mobile overflow

* Fix lint issues

* Remove aria-label and update tests

* Fix attempt for E2E tests

* Update tests

* Refine E2E change
@zhaojj2209 zhaojj2209 self-assigned this Apr 2, 2023
@zhaojj2209 zhaojj2209 added the c.Bug Bug/defect report label Apr 2, 2023
@zhaojj2209 zhaojj2209 added this to the V8.26.0 milestone Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants