feat: DH-19307: External theme support#2425
Conversation
bmingles
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2425 +/- ##
==========================================
+ Coverage 47.06% 47.23% +0.16%
==========================================
Files 714 718 +4
Lines 39468 39594 +126
Branches 10063 10098 +35
==========================================
+ Hits 18576 18701 +125
+ Misses 20881 20839 -42
- Partials 11 54 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Most of the functions in this module were pre-existing and moved from @deephaven/app-util. The only new function is sendMessageToParent
bmingles
changed the title
…e-usable in DHE (DH-19307)
bmingles
force-pushed
the
DH-19307_external-theme-support
branch
from
ee8ffad to
0e3a4f8
Compare
|
|
||
| return ( | ||
| <ThemeProvider themes={themes}> | ||
| <ThemeProvider themes={themes} waitForActivation={waitForActivation}> |
There was a problem hiding this comment.
I don't think we need to pass this waitForActivation logic down to ThemeProvider, and we don't need the waitForActivation here.
We're already doing themes = useCustomThemes which internally is already waiting for the external theme if it's activated.
I would think at most here we'd need:
{themes != null && <ThemeProvider> ...}Though maybe I'm missing something.
There was a problem hiding this comment.
@mofojed The reason we need waitForActivation is due to the difference of plugin themes vs external themes.
- plugin themes - get loaded after login, so we have to be able to render children to show the login form while they are still null.
- external themes - don't require the login, so we could do the simple
themes != null && <ThemeProvider>you suggested. Plugin themes can't do this
I think you are right in that we don't have to pass it down, but as far as I can tell, we do need some mechanism to differentiate.
I'll update accordingly and add more comments, unless you are seeing a better way.
There was a problem hiding this comment.
Actually, looks like we do need waitForActivation in ThemeProvider since the condition can't be determined from the passed in themes prop. I've added a more detailed comment to the waitForActivation prop.
| useEffect(() => { | ||
| if (!result.isEnabled) { | ||
| return; | ||
| } | ||
|
|
||
| logger.debug('Requesting external theme data'); | ||
|
|
||
| /** Parse external theme data and update the result */ | ||
| function handleExternalThemeData(externalThemeData: ExternalThemeData) { | ||
| const themeData = parseExternalThemeData(externalThemeData); | ||
|
|
||
| setResult({ | ||
| isEnabled: true, | ||
| isPending: false, | ||
| themeData, | ||
| }); | ||
| } | ||
|
|
||
| /** Parent or current Window can explicitly set the theme */ | ||
| function onMessage(event: MessageEvent<PostMessage<unknown>>): void { | ||
| const parent = getWindowParent(); | ||
|
|
||
| // Allow messages from parent or current window | ||
| if (event.source !== window && event.source !== parent) { | ||
| return; | ||
| } | ||
|
|
||
| if (event.data.message === MSG_REQUEST_SET_THEME) { | ||
| if (isExternalThemeData(event.data.payload)) { | ||
| handleExternalThemeData(event.data.payload); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener('message', onMessage); | ||
|
|
||
| /** Request initial theme data from parent window */ | ||
| requestExternalThemeData() | ||
| .then(handleExternalThemeData) | ||
| .catch(err => { | ||
| logger.error(err); | ||
| setResult({ isEnabled: true, isPending: false }); | ||
| }); | ||
|
|
||
| return () => { | ||
| window.removeEventListener('message', onMessage); | ||
| }; | ||
| }, [result.isEnabled]); |
There was a problem hiding this comment.
I'd probably refactor this effect to make it more concise... the handleExternalThemeData doesn't need to be within it...
const handleExternalThemeData = useCallback((externalThemeData: ExternalThemeData) => ...);
useEffect(function startListeningWindowMessages() {
function onMessage(...) { ... }
window.addEventListener('message', onMessage);
return () => { window.removeEventListener( .... );
}, [handleExternalThemeData]);
useEffect(function initializeExternalTheme() {
...
request.ExternalThemeData().then(handleExternalThemeData).catch(...)
}, [handleExternalThemeData]);| export interface Response<T = unknown> { | ||
| id: string; | ||
| payload: T; | ||
| } | ||
|
|
||
| export function isMessage(obj: unknown): obj is Message { | ||
| const message = obj as Message; | ||
| return ( | ||
| message != null && | ||
| typeof message.id === 'string' && | ||
| typeof message.message === 'string' | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Removing all this stuff is a breaking change. Deprecate it instead (and re-export from utils where possible).
2 participants
DH-19307: External theme support via
postMessageapistheme=[EXTERNAL_THEME_KEY]query string paramMSG_REQUEST_GET_THEMEpostMessageMSG_REQUEST_SET_THEMEpostMessageTesting
You can download this html file: https://gist.github.com/bmingles/02451e09d218cdbe61d8c506e556562b . You should be able to open it in browser from local filesystem, no need for server.