search

jwplayer / ott-web-app

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

Feat / Restructure and a11y #435

Closed ChristiaanScheermeijer closed 8 months ago

ChristiaanScheermeijer commented 8 months ago

Description

Sorry in advance for dropping this PR 😅...

I initially planned to rebase the changes from the develop branch for a linear history, but unfortunately, there were too many commits and conflicts.

We talked about restructuring the OTT Web App for multi-platform purposes a few weeks ago. A lot of code can be reused for different platforms, but because it is currently a single package (web platform), it's hard to extract the code for a, let's say React Native app.

Most of the structure changes you've seen and reviewed already. We've worked hard for the last 3 weeks to finalize the restructure by cleaning up dependencies and ensuring the common package doesn't use browser typings or React.

Besides that, we've also addressed a lot of accessibility bugs. We aim to achieve at least WCAG 2.1 AA, but we're not there yet. In the next sprint we will address even more accessibility issues and submit this later as PR(s). The accessibility fixes are mainly applied to components and SCSS files.

The most significant changes made since you last reviewed the restructure are:

  1. Remove React and browser typings from the @jwp/ott-common package
  2. Move all SVG icons to the @jwp/ott-theme package
  3. Refactor the query param utils (we had multiple overlapping utils for the same purpose)
  4. Remove react-router from the @jwp/ott-common and @jwp/ott-hooks-react packages
  5. Remove react-query from the @jwp/ott-common package (see below)
  6. Refactor the integration registration to allow registering custom integrations more easily
  7. Show a nice player error when the media is GEO blocked for the user

We also addressed some issues found while testing:

  1. Cleeng login is really slow due to an expected failing listProfiles query with a few retries
  2. Paywall not working correctly for TVOD offers in the inline player variant
  3. User loading state never reset after logout or account deletion

We have found a few more problems but didn't have the time to fix these in this sprint. We've confirmed that these bugs are currently also present in the latest release.

There is 1 more PR open (almost ready to merge) that fixes a problem we've had while testing but was harder to fix, unfortunately.

https://github.com/Videodock/ott-web-app/pull/31

PS. You can also view each PR (with reviews) separately: https://github.com/Videodock/ott-web-app/pulls?q=is%3Apr+is%3Aclosed

In the next sprint, we'll continue working on more a11y issues and refactoring some services, controllers, containers, and hooks. Specifically, we want to move most logic from components, containers and hooks to services/controllers.

ChristiaanScheermeijer commented 8 months ago

Thank you very much for reviewing @dbudzins! I'm glad to hear that it wasn't too bad 😄

I think all your comments make sense. I will work on that soon. Do you want to do that before merging this PR, or can I submit a separate PR to develop?

ChristiaanScheermeijer commented 8 months ago

@dbudzins I've merged develop and also created a PR in our fork with some documentation updates:

https://github.com/Videodock/ott-web-app/pull/56

dbudzins commented 8 months ago

Thank you very much for reviewing @dbudzins! I'm glad to hear that it wasn't too bad 😄

I think all your comments make sense. I will work on that soon. Do you want to do that before merging this PR, or can I submit a separate PR to develop?

I think it would be best if we have at least a rough version of an overview doc included with the first merge, in case anyone tries to grab the latest and start working with it, they'll be able to navigate the basics. It doesn't have to be a full, in depth doc, we can always add and refine later.

ChristiaanScheermeijer commented 8 months ago

@dbudzins, the docs are updated to reflect the directory changes. I've also added an initial version of the workspaces documentation.

I've prefixed three workflows that are exclusively web only. The only questionable workflow is release-build-tag-release.yml which creates web build artifacts. But this workflow should only be used once because it also creates a tag and release.

Let me know what you think! 😄