-
Notifications
You must be signed in to change notification settings - Fork 356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Wrap older modals in theme class [WEB-1824] #8432
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update the GalleryModal?
This fixes the Hyperparameter Search modal, but it actually looks like the "Use in Notebook" modal doesn't use the determined So I think we need to apply the theme class to any direct imports of antd Modals, or use the |
@johnkim-det I think we had the same idea to search - the latest commit extends this to other uses of Antd Modal except for the Omnibar help (because it's not a function component and can't call useTheme...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@mapmeld Can you make a ticket to migrate these? |
@ClaireNeveu ok I made 3 tickets, because some are more complex than others |
Description
Applies system/light/dark theme to hyperparameter, notebook, or any old-style modal still inside
src/hooks/useModal
Test Plan
Switch to dark mode.
In a single-trial experiment, open the Hyperparameter tab, then click the Hyperparameter Search button
The background should be solid, and the title font-family should come from a CSS variable
Checklist
docs/release-notes/
.See Release Note for details.