Skip to content

Conversation

@kpuriIpac
Copy link
Contributor

Ticket: https://jira.ipac.caltech.edu/browse/FIREFLY-1744

  • The main reason for this bug is that DialogRootContainer is essentially a separate root (from the main FireflyRoot)
  • So when toggling theme, a re-render would not occur inside dialogs
    • But the rest of the dialog (except ADQL block) would still manage the theme change, because most of the dialog UI uses global Joy UI CSS
      • The ADQL block, though, uses Prism whose theme is injected via CSS strings, so it required an explicit re-render
        • the code changes in AppConfigDrawer and DialogRootContainer take care of this, forcing a re-render of the dialog on theme change
  • Even after the re-render of the dialog, useColorMode() in CssStrThemeWrapper would still not give the correct theme for the dialog (because dialogs are rendered in a separate React root with their own Joy CssVarsProvider)
    • This led to relying on getting the Joy scheme from the DOM in the case of dialogs (with ADQL block) specifically

Eventually, this PR could be made redundant if dialogs were not in a separate React Tree. We plan on discussing this change January 27-28.

Testing:
Firefly: https://fireflydev.ipac.caltech.edu/firefly-1744-adql-toggle-bug/firefly

  • test toggling dark to light mode and vice-versa upon opening a TAP Job Info dialog from the Job Monitor (described in ticket above)
    • ensure the ADQL block changes to dark/light mode as expected when you toggle the theme
  • test toggling dark to light mode and vice-versa on the Edit ADQL page (regression testing)

@kpuriIpac kpuriIpac added this to the 2026.1 milestone Jan 20, 2026
@kpuriIpac kpuriIpac self-assigned this Jan 20, 2026
@kpuriIpac kpuriIpac added the bug label Jan 20, 2026
Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested! Working as expected.

I think it's good as an intermediate fix/workaround.

<ListBoxInputFieldView {...{
onChange: (ev, newValue) => {
setMode(newValue);
DialogRootContainer.refreshDialogs();//forces a re-render of open dialogs to update their color mode
Copy link
Member

@jaladh-singhal jaladh-singhal Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's possible to directly pass this color mode (newValue) to dialog root here (like setMode/useMode does for firefly root) which would have prevented reading scheme from DOM.

Copy link
Contributor Author

@kpuriIpac kpuriIpac Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaladh-singhal I made a new commit that sort of follows this feedback. Can you take a look? I do think it's cleaner (I'll get rid of the commented out code if we decide to keep this change):

  • essentially, refreshDialogs(newValue) forces the dialog root to re-render.
  • The key in FireflyRoot (in reRender) forces a remount of FireflyRoot in the dialog root (which was previously missing, only the main (non-dialog) FireflyRoot was re-rendering, which is why useColorMode inside CssStrThemeWrapper would not give the right value for dialogs).
    • with this change, we don't need to make any changes in PrismADQLAware or CssStrThemeWrapper at all.

Let me know what you think.

@kpuriIpac kpuriIpac force-pushed the FIREFLY-1744-adql-toggle-bug branch from 8f490b9 to b933d5d Compare January 27, 2026 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants