Skip to content
This repository was archived by the owner on Jan 13, 2023. It is now read-only.

Comments

Dark mode#70

Open
hylophile wants to merge 64 commits intomixnjuice:masterfrom
hylophile:dark-mode
Open

Dark mode#70
hylophile wants to merge 64 commits intomixnjuice:masterfrom
hylophile:dark-mode

Conversation

@hylophile
Copy link

@hylophile hylophile commented Mar 19, 2020

Adds a ToggleButton to user settings to toggle Dark Mode.
Can easily be refactored for more themes.

Still to do:

  • complete the dark mode theme
    • cards
    • tables
    • togglebutton background too dark
    • hr feels too bright (on page bottom)
    • toasts
  • add persistency of the setting (save in db or localStorage?)
  • add tests
    • redux store
  • rename color variables
    e.g. lighter-blue, light-blue become blue-lighter and blue-light
  • add variables for newly introduced colors
  • get rid of gray("200") et al.
  • fix white flare-up in input group when changing theme if possible
    "fixed" by instead adding global transition when changing color theme
  • add better comments to new style rules
  • try to get rid of the mixin duplication

Other things:

  • split style into multiple files for better overview
  • the hover on the dropdown menu is slightly off to the right
  • upload button height is a bit off

@ayan4m1 ayan4m1 requested review from a team and ayan4m1 and removed request for a team March 20, 2020 00:52
@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #70 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #70   +/-   ##
=======================================
  Coverage   78.89%   78.89%           
=======================================
  Files          87       87           
  Lines        1668     1668           
=======================================
  Hits         1316     1316           
  Misses        352      352           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2bbebc...e2bbebc. Read the comment docs.

});

export default withRouter(connect(null, mapDispatchToProps)(App));
export default withTheme(withRouter(connect(null, mapDispatchToProps)(App)));
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

it('reduces TOGGLE_DARK_MODE action', () => {
const action = actions.toggleDarkMode();

expect(reducer({}, action)).toEqual({ name });
Copy link
Contributor

Choose a reason for hiding this comment

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

It would improve this test if it also tested that the theme is set to dark if toggleDarkMode is called again.

const store = mockStore({ application: initialState });
const store = mockStore({
application: initialApplicationState,
theme: initialThemeState
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any specific reason for theme to live outside application?

Copy link
Author

Choose a reason for hiding this comment

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

As we now don't have a theme component anymore, not really. I moved all theme-related redux code into application now, i.e. including the selectors/reducers/actions. The only issue is that the only place where the toggleDarkMode() action is used, on the user settings page, all application actions are imported. I don't know if that's an issue though (if it gets optimized away or if it would be better to only import one action or move the action to a different file).

<Row className="text-center">
<Col md="12" className="align-self-center">
Toggle Dark Mode
<ToggleButton
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants