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

[#801] Fix/close menus with escape button #417

Merged
merged 4 commits into from
Feb 13, 2023

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented Jan 5, 2023

No description provided.

@vaszig vaszig marked this pull request as draft January 5, 2023 13:56
@vaszig vaszig force-pushed the fix/801-use-escape-for-closing-menus branch from 0f94d22 to 687ec7d Compare January 11, 2023 08:04
@vaszig vaszig marked this pull request as ready for review January 11, 2023 08:04
@Bartvaderkin
Copy link
Contributor

@vaszig I think this has been covered by Sven's solution for some other issues like the focus handling (the menu is now a )

@vaszig
Copy link
Contributor Author

vaszig commented Jan 11, 2023

@Bartvaderkin thanks for letting me know. I will double check this as soon as Svenv's PRs are merged.

@alextreme
Copy link
Member

@vaszig can be checked, Svens PR was merged afaik

@alextreme alextreme requested a review from jiromaykin February 5, 2023 14:58
@alextreme
Copy link
Member

Discussed: This is for being able to close (for instance) the dropdown menu in Mijn Profiel with an escape-button

image

@jiromaykin
Copy link
Contributor

jiromaykin commented Feb 10, 2023

@vaszig This seems to work well (and it doesn't even have the Header click-trap problem) but when I check out this branch the dropdown seems to be styled differently now (extra borders in different places)?
(Edit): It seems its inherited the border-width of the "button--secondary " style rule.

.button--secondary { background-color: var(--color-secondary); border: 1px solid var(--color-secondary); [.....]

accountmenu

@vaszig
Copy link
Contributor Author

vaszig commented Feb 10, 2023

It's an old branch so the borders are not updated here.
It works because it only implements what we wanted..the escape key. Svenv's approach is more generic and affects a lot of code, that's why we have the problem.

@jiromaykin
Copy link
Contributor

jiromaykin commented Feb 10, 2023

It's an old branch so the borders are not updated here.

Sorry @vaszig yes, I see now - I would think this PR might be closed/reviewed, once #466 (escape-button combo with focus-trap on header/search) is reviewed and merged?

@vaszig
Copy link
Contributor Author

vaszig commented Feb 10, 2023

Sorry @vaszig yes, I see now - I would think this PR might no longer be needed, once #466 (escape-button combo with focus-trap on header/search) is reviewed and merged?

Yes indeed, we can close this PR as soon as we merge 466. We can handle the dropdowns (task 1078 in taiga) in a separate PR.

@jiromaykin jiromaykin force-pushed the fix/801-use-escape-for-closing-menus branch from 687ec7d to b5f08e8 Compare February 10, 2023 15:40
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Merging #417 (2018977) into develop (9e8c1bd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #417   +/-   ##
========================================
  Coverage    96.46%   96.46%           
========================================
  Files          505      505           
  Lines        18255    18255           
========================================
  Hits         17609    17609           
  Misses         646      646           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@jiromaykin jiromaykin left a comment

Choose a reason for hiding this comment

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

The Dropdown JS we still need; the PrimaryNavigation might not be necessary anymore after all of the updates from PR #466

@vaszig vaszig requested a review from jiromaykin February 13, 2023 10:53
@alextreme alextreme merged commit ab6e71b into develop Feb 13, 2023
@alextreme alextreme deleted the fix/801-use-escape-for-closing-menus branch February 13, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants