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

feat(@clayui/focus-trap): Add FocusTrap component #5427

Merged
merged 6 commits into from
Apr 4, 2023

Conversation

veroglez
Copy link
Member

@veroglez veroglez commented Mar 17, 2023

It fixes: #5409

Hi @matuzalemsteles! Here is the draft for the new FocusTrap component. I have proposed the following: the component has an active props, which is in charge of activating the focus trap. In this way, it's the responsibility of the user to activate the focus trap. It could be always activated or, for example, activate it with the click of a button.

I have also added stories for Storybook to verify its behavior (video attached). What do you think?

focusTrap.mov

TO DO:

  • tests
  • doc

@veroglez veroglez force-pushed the issue-5409 branch 2 times, most recently from 95ed7e8 to 90b18ff Compare March 27, 2023 17:59
@veroglez veroglez force-pushed the issue-5409 branch 2 times, most recently from 8c82f82 to 6aae847 Compare March 29, 2023 18:41
Copy link
Member

@matuzalemsteles matuzalemsteles left a comment

Choose a reason for hiding this comment

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

@veroglez looks good now, I just added a comment of a small detail but looks good to go ahead now. Thanks for working on it.

packages/clay-core/src/focus-trap/FocusTrap.tsx Outdated Show resolved Hide resolved
@veroglez veroglez marked this pull request as ready for review March 30, 2023 16:34
@veroglez
Copy link
Member Author

veroglez commented Apr 4, 2023

Hi @matuzalemsteles! I have added two commits with the tests and documentation (3c8a1df, af2e8f6) , could you take a look? Thanks in advance!

Copy link
Member

@matuzalemsteles matuzalemsteles left a comment

Choose a reason for hiding this comment

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

LGTM! I send a commit to add the functionality to suppress elements outside the scope of the focus trap, so it will work better when using the mouse for example by clicking elsewhere on the page, the focus will go back into scope again.

@matuzalemsteles matuzalemsteles merged commit 6c1a503 into liferay:master Apr 4, 2023
2 checks passed
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

3 participants