ott-web-app icon indicating copy to clipboard operation
ott-web-app copied to clipboard

Refactor / Layout and AppRoutes cleanup

Open ChristiaanScheermeijer opened this issue 10 months ago • 2 comments

Description

We noticed that when adding platforms, we hoisted the AppRoutes and Layout containers to change the routing and main layout with platform-specific changes. However, these currently contain lots of logic and conditionals, which feel redundant and error-prone when syncing upstream changes.

I might have gone too far with this PR, but the idea was to split more logic into smaller containers. The Header component was way too big and received way too many props. I could simply wrap the Header component in a container, but then we still need to hoist all this logic when making a small change to the header for a platform. For example, when we don't want the user menu or actions.

The size of the AppRoutes and Layout containers is an essential part of this PR:

https://github.com/jwplayer/ott-web-app/blob/78ec79bc2ed0a28fd28b5050faf799a1364e4714/platforms/web/src/containers/AppRoutes/AppRoutes.tsx

https://github.com/jwplayer/ott-web-app/blob/78ec79bc2ed0a28fd28b5050faf799a1364e4714/packages/ui-react/src/containers/Layout/Layout.tsx

Steps completed:

According to our definition of done, I have completed the following steps:

  • [x] Acceptance criteria met
  • [ ] Unit tests added
  • [ ] Docs updated (including config and env variables)
  • [ ] Translations added
  • [x] UX tested
  • [x] Browsers / platforms tested
  • [x] Rebased & ready to merge without conflicts
  • [ ] Reviewed own code

ChristiaanScheermeijer avatar Apr 16 '24 10:04 ChristiaanScheermeijer

Visit the preview URL for this PR (updated for commit 0d3e3f7):

https://ottwebapp--pr501-feat-ui-cleanup-2zwpy5tj.web.app

(expires Fri, 24 May 2024 17:55:37 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

github-actions[bot] avatar Apr 16 '24 10:04 github-actions[bot]

Moving forward, a change like this is necessary. I see some minor improvements we could apply in the CSS selectors, but that can always be improved upon in a later phase. I hope we can merge this ASAP.

langemike avatar Apr 17 '24 07:04 langemike

@ChristiaanScheermeijer please resolve the conflicts and I will merge it.

AntonLantukh avatar Apr 24 '24 12:04 AntonLantukh

@AntonLantukh this branch is rebased and squashed (making it easier to rebase)

ChristiaanScheermeijer avatar Apr 24 '24 17:04 ChristiaanScheermeijer