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

[Outreachy Task Submission] Focus Trap added To Onboarding Modal #996

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

MugoBrian
Copy link
Contributor

@MugoBrian MugoBrian commented Apr 1, 2024

This PR addresses this issue.

Fix:

  1. Implemented the cdkFocusTrap directive to ensure focus trapping functionality within the onboarding modal.
  2. Updated the Help&Support text by wrapping it within a button semantic element to make it focusable.
  3. Adjusted its color and hover color to improve color contrast for better accessibility.

To Reproduce
Steps to reproduce the behavior:

  1. Navigate to the Ushahidi platform.
  2. Access the browser's developer tools console and clear or modify the value of "USH_is_onboarding_done" to true in the local storage. Afterward, refresh the page.
  3. Utilize the tab key on your keyboard to navigate through the elements on the page.

Screencast
FocusTrap Fix for Onboarding Modal.webm

@MugoBrian
Copy link
Contributor Author

Hello @Angamanga, @Ifycode , @tuxpiper , this PR is ready for review.

@Chiemezuo
Copy link
Contributor

Great catch, buddy!!
However, it might have been better if you raised a PR to just close the issue, and raised another one to show the other fix. This way, it could be reviewed quicker, without anyone having to bother about whether or not they want to introduce that particular piece of CSS into the codebase.

@MugoBrian
Copy link
Contributor Author

Great catch, buddy!! However, it might have been better if you raised a PR to just close the issue, and raised another one to show the other fix. This way, it could be reviewed quicker, without anyone having to bother about whether or not they want to introduce that particular piece of CSS into the codebase.

Hello @Chiemezuo, I don't quite get it. That means I will have two PRs address same issue or?

@Chiemezuo
Copy link
Contributor

No, I meant: A PR to address the direct issue, which is the lack of keyboard focus trap.
And then another one to improve the design.

This way, the first can be approved/merged without the second getting in the way.
It was just a suggestion though (for faster review time), but there's nothing wrong whatsoever with your current approach.

@MugoBrian
Copy link
Contributor Author

No, I meant: A PR to address the direct issue, which is the lack of keyboard focus trap. And then another one to improve the design.

This way, the first can be approved/merged without the second getting in the way. It was just a suggestion though (for faster review time), but there's nothing wrong whatsoever with your current approach.

If I am getting this right, do you mean I need to reference the issue in my PR?

@Chiemezuo
Copy link
Contributor

No, not at all.
I was saying: The third problem you solved in this PR (as written in your description) is good enough as a standalone PR.

@MugoBrian
Copy link
Contributor Author

No, not at all. I was saying: The third problem you solved in this PR (as written in your description) is good enough as a standalone PR.

Ooh. Haha :) I get you now.

@Chiemezuo
Copy link
Contributor

Cheers
:)

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.

None yet

2 participants