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

[DRAFT] added prototype styling for dark theme support #254

Merged
merged 34 commits into from
Feb 18, 2023

Conversation

bugy
Copy link

@bugy bugy commented Mar 23, 2022

Proposed changes

Here is a prototype of basic dark theme support. Theme variables are defined in sass/components/_theme_variables.css

By default :root variables are applied. If a theme = dark attribute added to document, then dark variables are picked up. In this way it would be possible to have even more than 2 themes per application if needed

I decided not to use css variables in scss files directly, but put them in scss variables (e.g. $bg-color: var(--background-color) !default;), this would be less breaking for users, who work with custom scss variables in their projects

I also adjusted tab/switch styling to better reflect material UI guidelines (and to use fewer variables).

If we would like to continue with this approach, I would kindly ask to merge #85 first, because there are quite some intersecting changes

Here is the testbed, which I used: https://materialize-testbed.s3-us-west-2.amazonaws.com/dark-theme/index.html#!
You can find theme switch in the right upper corner

The switch theme code looks like this:

      if (darkTheme) {
        document.documentElement.setAttribute('theme', 'dark');
      } else {
        document.documentElement.removeAttribute('theme');
      }

Screenshots (if appropriate) or codepen:

image
image

Types of changes

It might be a breaking change for users, who override sass variables.
Also, it would change a look of some components (to better match material UI guidelines)

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to change).

Checklist:

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@bugy bugy changed the title Dark theme 2 [DRAFT] added prototype styling for dark theme support Mar 23, 2022
@bugy bugy marked this pull request as draft March 23, 2022 13:15
@bugy
Copy link
Author

bugy commented Mar 23, 2022

New changes:

  • did some refactoring according to changes from other PRs
  • added new components support
  • ⚠️ aligned primary/secondary color to reflect material design guidelines: primary color is used everywhere, including tabs, links, buttons. Secondary color is used for very specific cases (e.g. floating button)
  • I changed default color for dark theme in order to make it easier to find some non-updated parts of code. The color will be set to default one before merging

@bugy bugy reopened this Mar 23, 2022
@DanielRuf DanielRuf requested a review from a team October 29, 2022 16:09
@DanielRuf
Copy link

@materializecss/members we need some reviews.

@Smankusors
Copy link
Member

wait, is this still draft or no? 😅

@wuda-io
Copy link
Member

wuda-io commented Nov 2, 2022

This is awesome, this feature should be merged and then continued from there. I suggest to merge into the v2 branch.
Also a toggle-button in the docs would be cool. The testbed is nice. It doesnt look like a draft anymore. Maybe we can add another website for v2 or just move to to the next release.

@bugy
Copy link
Author

bugy commented Nov 3, 2022

Hi @Smankusors to be honest, I don't fully remember the state here 😅
Probably not all the components were covered.

But I would still like to get feedback on the code changes and restyling of existing components (see #254 (comment))

wuda-io
wuda-io previously approved these changes Nov 3, 2022
Copy link
Member

@wuda-io wuda-io left a comment

Choose a reason for hiding this comment

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

LGTM, maybe add some documentation.
Also a section how to adapt the variables or add another theme would be helpful, if that is possible. Otherwise, it looks nice!

@DanielRuf
Copy link

@bugy just as a sidenote for future PRs: best-.practice for commit messages is to use present-tense, like "add ...", "change ...", "implement ..." instead of "added ..." and so on.

@wuda-io
Copy link
Member

wuda-io commented Nov 13, 2022

Nice! This is getting lit 🔥

@wuda-io
Copy link
Member

wuda-io commented Nov 23, 2022

Hey @bugy! Nice commits, when is this ready to merge? Are all components covered?
What is missing to merge this? Thanks for your great effort!

@bugy
Copy link
Author

bugy commented Nov 23, 2022

Hi @wuda-io it's still work in progress. I don't have too much time, do doing tiny steps
At the moment I'm testing my changes on the main demo website and there are quite a lot of fixes/adjustments

Btw, with this refactoring, there comes a question: which components should have accent (secondary) color. As per material guideline, it should be FAB, sliders and switches. However I'm the current codebase it's quite random, i would say. Should i keep it as it is, or we use this opportunity to align color scheme?
In my current version, i used accent color only for floating action buttons

@bugy
Copy link
Author

bugy commented Nov 23, 2022

I think I'm done with the changes :)

Now we need to decide:

  • which colors should we use for primary/secondary in the dark theme
  • what to do with the accent color (see [DRAFT] added prototype styling for dark theme support #254 (comment))
  • who wants to create docs? And how to do it? :)
  • who wants to do regression testing? Especially to try that on their own project
  • should we keep dark theme switch on the demo page as it is? (left side navigation panel)

@DanielRuf who can answer those questions?

I built the material demo pages, feel free to test:
https://materialize-testbed.s3-us-west-2.amazonaws.com/material-demo/index.html
(dark theme loading takes some time and it's flickering on page loading. In order to avoid it, theme switch should be performed as a first script in .html. Now it's the last step. So we load the page and all the data first, and only after this we switch the theme. If I'm not wrong, this is a misbehaviour of demo, not css variables approach)

I used grunt release but for whatever reason, it seems to be an old version of the website, with original authors in about page 🤔

@wuda-io
Copy link
Member

wuda-io commented Jan 31, 2023

Hello @bugy
This is awesome! It looks nice and I would keep the colors as they are for now.
If you want, I can do some of the documentation.
I also would test it on my projects.
The switch is nice on the nav-panel on the left, but we need to improve the loading script.
(maybe split the script into 2 parts: a small script for the header and another to load the heavy parts later)

I asked the other members how we should handle the merging of this PR, since it would be a nice step forward.
Thanks for your effort and i hope we can bring this to life soon!

@wuda-io wuda-io marked this pull request as ready for review February 8, 2023 21:13
@wuda-io
Copy link
Member

wuda-io commented Feb 8, 2023

Oh shit! Sorry I accidently clicked on Ready for Review, I did not know that this changed the PR State. How can I undo this action?

Sorry @bugy I wanted to see the changes and would be ready to got them through if you are ready.
If you want, we can try to merge it together in the current dev branch.
Please be patient with me, I dont have that much experience with github like you guys. Thanks!

@Jerit3787
Copy link

@wuda-io there should be a button to convert to draft on the right pane :)
image

Comment on lines +103 to 108

// Made less specific to allow easier overriding
.secondary-content {
float: right;
color: $secondary-color;
color: $primary-color;
}
Copy link
Member

Choose a reason for hiding this comment

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

Whats here the usage of primary?

Copy link
Member

Choose a reason for hiding this comment

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

I saw that you changed that other places too.
I assume you first had primary and then adapted for a better appearance.
Or was this due to the material specs?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I tried to align everything to materialize specs, where they use primary color almost everywhere, and secondary color is used only as accent for important actions (like floating action button)

Comment on lines -56 to +67
p { color: $slider-bg-color-light; }
p {
color: rgba(255, 255, 255, 0.75);
}
Copy link
Member

Choose a reason for hiding this comment

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

Where is the class .slides set? From noUISlider? I did not find any usage for this class.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's from noUISlider

Copy link
Member

@wuda-io wuda-io left a comment

Choose a reason for hiding this comment

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

Pretty much lines to read 😆 Looks good after all!
I would merge that 👍

@wuda-io wuda-io merged commit 7c56189 into materializecss:v2-dev Feb 18, 2023
@wuda-io
Copy link
Member

wuda-io commented Feb 18, 2023

I will check it out at my local project and then see how we have to adapt the documentation for this.
Thanks @bugy for the awesome work!

@bugy
Copy link
Author

bugy commented Feb 18, 2023

Sry guys for being inactive here :) Didn't have time for it recently

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.

5 participants