jwplayer / ott-web-app

Reference implementation for JWP-powered apps
Apache License 2.0
69 stars 52 forks source link

Refactor / replace modal with HTML Dialog #505

Closed ChristiaanScheermeijer closed 1 month ago

ChristiaanScheermeijer commented 2 months ago

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 useModal hook 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.

github-actions[bot] commented 2 months ago

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

ChristiaanScheermeijer commented 2 months ago

^ not sure what happened here 🤔

ChristiaanScheermeijer commented 2 months ago

@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 😄

ChristiaanScheermeijer commented 2 months ago

@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.

We're testing this, but currently, the web app isn't working at all on (older) TV devices. PR #519 fixes this problem :-)

ChristiaanScheermeijer commented 2 months ago

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.

royschut commented 2 months ago

We have performed our manual testing workflow, including this PR and found 2 bugs that I have fixed just now:

AntonLantukh commented 1 month ago

@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?

ChristiaanScheermeijer commented 1 month ago

Hi @AntonLantukh, thanks! I've fixed the e2e issues. This PR is ready to be (squashed) merged 😄