Skip to content
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

Merged
merged 33 commits into from
Mar 16, 2020

Conversation

alexpaxton
Copy link
Contributor

@alexpaxton alexpaxton commented Mar 12, 2020

  • Create key in redux state to determine how to render dashboards
  • Write styles to override default (dark) mode when light mode is enabled
  • Create a light theme for view types
  • Add Light/Dark toggle to dashboard controls
  • @121watts did some cleanup

Screen Shot 2020-03-12 at 1 15 43 PM
Screen Shot 2020-03-12 at 1 14 53 PM

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Documentation updated or issue created (provide link to issue/pr)
  • Signed CLA (if not already signed)

@russorat
Copy link
Contributor

@sanderson FYI

@sanderson
Copy link
Contributor

Is this for dashboards only or the UI as a whole?

@alexpaxton alexpaxton requested a review from a team March 12, 2020 23:13
@alexpaxton alexpaxton requested review from TCL735 and removed request for a team March 12, 2020 23:13
@alexpaxton
Copy link
Contributor Author

@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

@TCL735 TCL735 requested review from a team and removed request for TCL735 March 12, 2020 23:14
@alexpaxton alexpaxton requested review from a team and ebb-tide and removed request for a team March 12, 2020 23:29
ui/src/App.tsx Outdated

const mstp = (state: AppState): StateProps => {
const {
app: {
ephemeral: {inPresentationMode},
persisted: {currentPage, theme},
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@121watts I talked to @ebb-tide about our approach here a bit, and we concluded that it's unnecessary to have currentPage stored in persisted state. What do you think about moving it to ephemeral or somewhere else?

Copy link
Contributor

@ebb-tide ebb-tide left a 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 ?

@121watts
Copy link
Contributor

@ebb-tide we don't need to persist the currentPage to local storage. It can be stored in memory, specifically the redux store.

Copy link
Contributor

@ebb-tide ebb-tide left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alexpaxton alexpaxton merged commit b0d459f into master Mar 16, 2020
@alexpaxton alexpaxton deleted the feat/light-mode-dashboards branch March 16, 2020 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants