-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(dashboards): enable optional light mode for dashboards #17232
Conversation
@sanderson FYI |
Is this for dashboards only or the UI as a whole? |
…fluxdb into feat/light-mode-dashboards
@sanderson just dashboards for now. In our user research we learned that light mode was most useful for getting screenshots of a dashboard for use in presentations, or when the dashboard is on a wall mounted display |
ui/src/App.tsx
Outdated
|
||
const mstp = (state: AppState): StateProps => { | ||
const { | ||
app: { | ||
ephemeral: {inPresentationMode}, | ||
persisted: {currentPage, theme}, |
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.
I'm confused why we have a separate state for determining the current page- isn't it available via router path?
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.
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.
I understand that the route (url) is a proxy for whether the dashboards
component is mounted, so tracking currentPage
state and route state is somewhat equivalent. But I don't see why currentPage state should be persisted to local state. Should it be in redux instead? What do you think @121watts ?
@ebb-tide we don't need to persist the |
611a2ed
to
1303234
Compare
…fluxdb into feat/light-mode-dashboards
…fluxdb into feat/light-mode-dashboards
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.
👍 thank you!
@@ -2,6 +2,7 @@ | |||
|
|||
### Features | |||
|
|||
1. [17232](https://github.com/influxdata/influxdb/pull/17232): Allow dashboards to optionally be displayed in light mode |
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.
👍
Light/Dark
toggle to dashboard controls