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

chore(Portal): Listen for Escape keypress using addEventListener instead of event-stack #4504

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

Conversation

dominikdosoudil
Copy link
Contributor

It is only one removed EventStack instance from Portal. I wanna go step by step with it just to know that I am not going some wrong direction.

There are few decisions I would like to explain:

  • I've put the event handling into the PortalInner. It seems to me like the right place as PortalInner is only mounted when Portal is open (-> listeners only added when portal is open as well as with EventStack component).
  • I've made onClose and closeOnEscape required in PortalInner because it seems to me that PortalInner is and should only be used inside the Portal itself. Therefor there should not be option for SUIR developer to forget prop drill it.

Overall, I looked into it a bit and I think that most of the listeners should be in the PortalInner as it has access to the ref which will be needed for other handlers. I should even make Portal itself a bit lightweight when closed.

I hope that it makes sense and does not disrespect some SUIR architecture.

Copy link

vercel bot commented Nov 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
semantic-ui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 21, 2024 11:12am

@dominikdosoudil
Copy link
Contributor Author

Btw there is some behaviour change in the "Multiple Modals" example.

Prod: when I click "Open first Modal", then "Proceed", then hit Escape both modals close. When I open the first one again, the second one cannot be opened. The dimmer is also darker so it seems that something opens twice.

This PR: Still both modals close with 1 Escape hit, however second opening of second modal works.

I believe that the production behaviour is a bug tho.

@dominikdosoudil
Copy link
Contributor Author

Test error is done() called multiple times in hook <"after each" hook> (of root suite); however that does not happen to me locally. Is that really some problem in the code or just test environment having a bad mood?

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.

1 participant