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

WebUI Dark Mode #8137

Merged
merged 13 commits into from
Sep 15, 2024
Merged

WebUI Dark Mode #8137

merged 13 commits into from
Sep 15, 2024

Conversation

itaigilo
Copy link
Contributor

@itaigilo itaigilo commented Sep 8, 2024

Closes #8118.

Change Description

Adds a Dark Mode to the WebUI,
Activated by a button (located at the right of the top-bar).

The user's selected mode is saved on the local-storage (of the browser), making it persistent per-browser.

Demo

Dark.Mode.mp4

Testing Details

Tested manually on Chrome and Firefox.

It still requires some more QA, to validate parts of the UI I'm less familiar with.

Additional info

Still open:

  • The quickstart page icons should be replaced with a transparent background.
  • In parquet rendering, the code-editor doesn't support dark-mode, so it seems ok to leave it as is.

@itaigilo itaigilo added area/UI Improvements or additions to UI include-changelog PR description should be included in next release changelog labels Sep 8, 2024
@@ -41,6 +41,7 @@
"react-router-dom": "^6.20.1",
"react-simple-code-editor": "^0.13.1",
"react-syntax-highlighter": "^15.5.0",
"react-toggle-dark-mode": "^1.1.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The UI component of the toggle button.

@@ -1,19 +1,24 @@
import React, {FC, useState} from "react";
import React, {FC, useContext, useState} from "react";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lint is inconsistent in the WebUI code -
I tried to avoid messing these, but in some files it was a bit too much.

);
return (
<Router>
<WithAppContext>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides lint changes, the only change in this file is adding wrapping with WithAppContext.

);
return !inline && hasLang ? (
<SyntaxHighlighter
style={state.settings.darkMode ? dark : syntaxHighlightStyle}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excluding lint, this line is the only change in this file.

Copy link

github-actions bot commented Sep 8, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed

Copy link

github-actions bot commented Sep 8, 2024

E2E Test Results - Quickstart

10 passed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using named colors, the data-bs-theme knows how to handle dark-mode - which doesn't work when using #rrggbb colors.

I changed all the rgb colors to have the closest named color -
There are some left-overs which are ones with no close named color and that I couldn't find where they are used.

Copy link
Member

Choose a reason for hiding this comment

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

The color palette are brand colors. I think this requires the participation of @keren-lakeFS in this decision.
Please make sure that this is approved by marketing (due to changes of the default palette)

@itaigilo itaigilo requested a review from a team September 10, 2024 17:45
}

const initialLocalSettings: AppContext = {
darkMode: window.localStorage.getItem(localStorageKeys.darkMode) === String(true),
Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

const appContextReducer = (state: AppContextType, action: Action) => {
switch (action.type) {
case AppActionType.setDarkMode:
window.localStorage.setItem(localStorageKeys.darkMode, String(action.value));
Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

Everything looks great.
The only thing that bothers me is that there's no separation between the navbar and the context window in the dark mode - otherwise I would have approved the PR

Copy link
Contributor

@guy-har guy-har left a comment

Choose a reason for hiding this comment

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

🕶️

@@ -222,7 +223,7 @@ const TagList = ({ repo, after, prefix, onPaginate }) => {
</ActionsBar>
{content}
<div className={"mt-2"}>
A tag is an immutable pointer to a single commit. <a href="https://docs.lakefs.io/understand/object-model.html#identifying-commits" target="_blank" rel="noopener noreferrer">Learn more.</a>
A tag is an immutable pointer to a single commit. <a href="https://docs.lakefs.io/understand/object-model.html#tags" target="_blank" rel="noopener noreferrer">Learn more.</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A tag is an immutable pointer to a single commit. <a href="https://docs.lakefs.io/understand/object-model.html#tags" target="_blank" rel="noopener noreferrer">Learn more.</a>
A tag is an immutable pointer to a single commit. <a href="https://docs.lakefs.io/understand/model.html#tags" target="_blank" rel="noopener noreferrer">Learn more.</a>

@@ -76,7 +77,7 @@ const TopNav = ({logged = true}) => {
);
}
return (
<Navbar variant="dark" bg="dark" expand="md">
<Navbar variant="dark" bg="dark" expand="md" className="border-bottom">
Copy link
Contributor Author

@itaigilo itaigilo Sep 15, 2024

Choose a reason for hiding this comment

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

@N-o-Z I've added the "border-bottom" for extra separation. It's un-noticeable in Light mode, so no real need for extra distinction here.

@itaigilo itaigilo enabled auto-merge (squash) September 15, 2024 10:36
@itaigilo itaigilo merged commit 9f790ab into master Sep 15, 2024
38 checks passed
@itaigilo itaigilo deleted the feature/ui-dark-mode branch September 15, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/UI Improvements or additions to UI include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI Dark mode
3 participants