Refactor / Layout and AppRoutes cleanup #501
Conversation
ChristiaanScheermeijer
requested review from
langemike,
dbudzins,
AntonLantukh,
MelissaDTH and
royschut
|
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 |
| useEffect(() => { | ||
| if (isLoggedIn && profilesEnabled && !profiles?.length) { | ||
| profileController?.unpersistProfile(); | ||
| } | ||
| // Trigger once on the initial page load | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
@AntonLantukh do you remember why this was needed? Can we move this to the controller instead?
There was a problem hiding this comment.
I don't really remember...It was right in the component, all I did was wrapping it into the useEffect hook. I think it may be connected with the case when you have a profile saved in the local storage but no profile on the back-end side. So probably this way we can solve the inconsistency problem.
There was a problem hiding this comment.
@kiremitrov123 maybe you know why this part was needed?
There was a problem hiding this comment.
My guess is that this is only needed when switching configs (between integration with and without profiles).
|
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. |
ChristiaanScheermeijer
force-pushed
the
feat/ui-cleanup
branch
from
5b086f2 to
96f6884
Compare
packages/ui-react/src/containers/HeaderLanguageSwitcher/HeaderLanguageSwitcher.tsx
Show resolved
Hide resolved
|
|
||
| import styles from './HeaderSearch.module.scss'; | ||
|
|
||
| const HeaderSearch = () => { |
There was a problem hiding this comment.
Should we also make it part of the Header component?
There was a problem hiding this comment.
This also is a container (although this one also contains UI...). The UI can be split into a component in the Header folder, but the container remains. We probably want to make a better/stricter convention for this
There was a problem hiding this comment.
Okay I see your point now, using components for all stateless UI and containers for stateful. My concern with the header is that it is split into several folders now across components and containers.
Maybe features folder could help us in the future where we would group some connected containers/components (Header (or Layout for Cards, Header and Footer), Form, EPG, Payments, Subscriptions ...)? Components could be just for something we reuse across features (maybe also stateful). Just thinking out loud now.
There was a problem hiding this comment.
Thanks, Anton! Yes, I remember @dbudzins also suggesting something similar. Let's discuss this in the near future! 😄
|
@ChristiaanScheermeijer please resolve the conflicts and I will merge it. |
ChristiaanScheermeijer
force-pushed
the
feat/ui-cleanup
branch
from
96f6884 to
0d3e3f7
Compare
|
@AntonLantukh this branch is rebased and squashed (making it easier to rebase) |
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
Headercomponent 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: