-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[ui] Settings page #20310
[ui] Settings page #20310
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @hellendag and the rest of your teammates on Graphite |
97d58cc
to
ae56972
Compare
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit ae56972. |
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit ae56972. |
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.
Code looks super clean! Just left one comment inline 🙌
import {useTrackPageView} from '../app/analytics'; | ||
import {useDocumentTitle} from '../hooks/useDocumentTitle'; | ||
|
||
const InstanceConfigStyle = createGlobalStyle` | ||
.CodeMirror.cm-s-instance-config { | ||
.CodeMirror.cm-s-instance-config.cm-s-instance-config { |
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.
Hmm the classnames are identical here? Not sure what this second one does in this context but maybe I've just never tried :-o
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.
Oh, this is just to add a little juice to the specificity to make sure it overrides the default CodeMirror styles. Class names can be repeated to bump the class count in the specificity calculation. :) Right now there's a tie, so we're ending up with whichever style loads last.
## Summary & Motivation Introduce consolidated Settings page, to be available at `/settings`. This is behind a (currently hidden) client-side flag. I'll expose it in User Settings once I have the Cloud side of this ready to go. Video in [Linear task](https://linear.app/dagster-labs/issue/FE-185/implement-new-nav-and-page-header-component). ## How I Tested These Changes Enable flag. - Verify that "Settings" appears in top nav. - Click Settings, verify that the pages and panes render properly, and that navigation behaves as expected. - Reload definitions, verify success.
Summary & Motivation
Introduce consolidated Settings page, to be available at
/settings
. This is behind a (currently hidden) client-side flag. I'll expose it in User Settings once I have the Cloud side of this ready to go.Video in Linear task.
How I Tested These Changes
Enable flag.