Refactor / replace modal with HTML Dialog #505
Conversation
ChristiaanScheermeijer
requested review from
langemike,
budzworthy,
AntonLantukh,
MelissaDTH and
royschut
|
Visit the preview URL for this PR (updated for commit 3fd7f4f): https://ottwebapp--pr505-feat-use-html-dialog-iooqd9g7.web.app (expires Wed, 12 Jun 2024 11:14:23 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c198f8a3a199ba8747819f7f1e45cf602b777529 |
|
^ not sure what happened here 🤔 |
|
@AntonLantukh @dbudzins, what do you think of this PR as an alternative to #485? I do have to test this (and the polyfill) for older browsers (TV browsers), but if you have a strong preference over this or #485, please let me know 😄 |
There was a problem hiding this comment.
@ChristiaanScheermeijer I like this approach. Do you think it will also work with TV devices? If it does, then I would proceed with this one.
| </div> | ||
| </div> | ||
| `; | ||
| exports[`<SideBar /> > renders sideBar closed 1`] = `<div />`; |
There was a problem hiding this comment.
Hm, all the content got removed. Is it expected?
There was a problem hiding this comment.
Yes, the Sidebar is now rendered in a modal/dialog which doesn't render when it is hidden.
There was a problem hiding this comment.
But... I found that the Sidebar open snapshot was also empty. This is because there is nothing rendered in the container due to the usage of portals in the Modal component. This is fixed now.
We're testing this, but currently, the web app isn't working at all on (older) TV devices. PR #519 fixes this problem :-) |
ChristiaanScheermeijer
force-pushed
the
feat/use-html-dialog
branch
from
ea37fc7 to
fee117a
Compare
ChristiaanScheermeijer
changed the base branch from
feat/modal-manager
to
develop
ChristiaanScheermeijer
dismissed
AntonLantukh’s stale review
The base branch was changed.
|
I've tested this on Chrome 68 (which we will support after #519), and it seems to work fine. The HTML dialog element is supported from Chrome 37 and up. |
|
We have performed our manual testing workflow, including this PR and found 2 bugs that I have fixed just now:
|
|
@royschut @ChristiaanScheermeijer looks good to me! I want to merge this PR if all the tests pass. Any objections? UPD: tests seem to fail, could you take a look? |
|
Hi @AntonLantukh, thanks! I've fixed the e2e issues. This PR is ready to be (squashed) merged 😄 |
* refactor: use html dialog elements * refactor(player): fix endless unmount loop * refactor: re-fix backdrop click to close modal * refactor(project): use events for animation open close callbacks * test(e2e): fix close modal by backdrop test * test(e2e): fix timeout on choose offer modal * test(e2e): skip click outside modal test for mobile --------- Co-authored-by: Roy Schut <[email protected]>
Description
This PR is an alternative to #485. In the modal manager PR, we try to manage everything ourselves, which gives us ultimate control but is risky and needs to be maintained in the future.
The HTML Dialog element has been out for some time now, but I actually never used it because of the browser support. But looking at the support table, I don't see why we shouldn't use it. Over 96% of the global browsers support the HTML Dialog element.
Using the native dialog element removes the need for the ModalProvider, because we can use the DOM to close open modals.
Only the body scrolling was still possible so I modified the
useModalhook to hide the scrollbars when a modal is open.Although we don't have to manage focus anymore, I did find one edge case when opening the account modal from the sidebar. Because the last focused element doesn't exist anymore (I fixed this in the ModalProvider), the body receives focus when closing the last modal.
This is a proposal PR, so let me know if you have any feedback or criticism.