fix: theme flickering#16903
Conversation
dosubot
Bot
added
ui
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (10/02/24)1 reviewer was added to this PR based on Keith Williams's automation. |
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
This works - but it also breaks the Booker theme for the logged in user - which is meant to take priority over the dashboard theme.
E2E results are ready! |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
@emrysal Follow up to our discussion: I improved the vanilla JS logic inside the inline script to take the booking page theme into consideration. I tested and all works fine. Let me know it's the same case in your end! |
|
@hbjORbj FYI it's all good from the booking's end, just waiting for @hariombalhara to confirm embeds. |
|
@hariombalhara Can you give this a review? |
| (function applyTheme() { | ||
| try { | ||
| const appTheme = localStorage.getItem('app-theme'); | ||
| if (!appTheme) return; | ||
|
|
||
| let bookingTheme, username; | ||
| for (let i = 0; i < localStorage.length; i++) { | ||
| const key = localStorage.key(i); | ||
| if (key.startsWith('booking-theme:')) { | ||
| bookingTheme = localStorage.getItem(key); | ||
| username = key.split("booking-theme:")[1]; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| const onReady = () => { | ||
| const isBookingPage = username && window.location.pathname.slice(1).startsWith(username); | ||
|
|
||
| if (document.body) { | ||
| document.body.classList.add(isBookingPage ? bookingTheme : appTheme); | ||
| } else { | ||
| requestAnimationFrame(onReady); | ||
| } | ||
| }; | ||
|
|
||
| requestAnimationFrame(onReady); | ||
| } catch (e) { | ||
| console.error('Error applying theme:', e); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Could we add some unit tests for this. We could make it a function and use fn.toString() here.
This logic works in combination with app-providers-app-dir.tsx. So, if possible we should share constants b/w them e.g. 'booking-theme' prefix.
Also, I am not clear why we need to add this logic here. Doesn't AppProviders stuff work now?
There was a problem hiding this comment.
No; at the moment there's a brief moment between pageload and app render where it decides on the wrong theme (only when dark mode is selected)
5 participants
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist