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

M3 styles #409

Merged
merged 7 commits into from
Sep 1, 2023
Merged

M3 styles #409

merged 7 commits into from
Sep 1, 2023

Conversation

danice
Copy link

@danice danice commented Aug 17, 2023

Proposed changes

As commented in discussion #397, this updates _theme_variables.scss to use variables coming from m3 theme builder website.
This way new styles can be created just replacing tokens.module.scss with the file generated by the site (there is an example on a new theme in the 2nd commit of this PR).

IMPORTANT: this looks quite well in the presented examples, but is not completed. Some variables from _theme_variables.scss have constant values and should be replaced by values using M3 variables. This should be done checking how this variables are used in MaterializeCSS and what is the corresponding variable following M3 color indications.

NOTE: I see this PR, more like a way to discuss and doing proofs than for actually using it as-is.

Screenshots:

See discussion #397 for screenshots.

Types of changes

SCSS Styles

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change: this will change the styles of the website and for anyone using directly _theme_variables.scss styles

Checklist:

  • I have read the CONTRIBUTING document.
  • My commit messages follows the conventional commit format
  • My change requires a change to the documentation, and updated it accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@danice danice mentioned this pull request Aug 17, 2023
8 tasks
@wuda-io
Copy link
Member

wuda-io commented Aug 20, 2023

Yes this is awesome! I also had this in mind, so thanks for making progress.
Why not using it as it-is? We are still in alpha so i think thats ok.
We should merge and then clean out the details.

@bugy
Copy link

bugy commented Aug 27, 2023

Hey, thanks for the heavy lifting, @danice!

I would have multiple comments here, also to @wuda-io:

  • Do we want to do high-coupling to the m3 builder? I think it would be better, to define a list of materializecss relevant variables and then assign them to m3 variables by default. So that if someone wants to override those variables for his project, he doesn't have to override unneeded m3 variables
  • I see that some existing styles were changed. I tried to align them carefully so that they look as per material design specifications. Did someone check the updated colors/fonts/components, so that they still make sense and look as per specs?
  • When looking at the screenshots from the original discussion, I think something is wrong there: text is barely readable there:
    image
    image
    image

It could be, that I used improper variables for defining font OR background color. Or I forgot to set dynamic color for the font there at all. But this should be fixed, I think.

@danice
Copy link
Author

danice commented Aug 28, 2023

Do we want to do high-coupling to the m3 builder? I think it would be better, to define a list of materializecss relevant variables and then assign them to m3 variables by default. So that if someone wants to override those variables for his project, he doesn't have to override unneeded m3 variables

I agree, we are just using existing materializecss variables and assigning them to the m3 variables, and I think we should continue this way, with maybe some minor adjustments in the variables.

When looking at the screenshots from the original discussion, I think something is wrong there: text is barely readable...

This is also happening in current website, just that is less visible. The problem is that this rule

.sidenav li > a:not(.btn):not(.btn-large):not(.btn-small):not(.btn-flat):not(.btn-large):not(.btn-floating) {
  color: var(--font-color-main);
}

is overwritting the

.sidenav li.active > a:not(.collapsible-header) {
  color: var(--font-on-primary-color-main);
  background-color: var(--primary-color);
}

and then --font-color-main variable is applied instead of --font-on-primary-color-main that will be the correct one. I will try to prepare a fix for that.

@bugy
Copy link

bugy commented Aug 28, 2023

@danice thanks for the response. Regarding "existing materializecss variables", fair, you are right, I misunderstood the changes.
However these 2 files look suspicious:

"components/colors.module"
"components/typography.module"

We expose global styles (selectors), which depend on md variables. I guess those files come from m3, but these selectors are not part of materializecss spec, I think. And they can conflict.

I would suggest removing those files and including only the files with variables.

This is also happening in current website, just that is less visible.

👍 Probably I overlooked it when I refactored the variables. Thanks!

PS could you remove the commented out code, please?

@danice
Copy link
Author

danice commented Aug 28, 2023

You are right in that this files colors.module and typography.module are not needed, as we just need the variable definitions. So we could perfectly remove that files for the components M3 theme(s) task.

colors.module is just a repetitive list of classes for applying M3 variables:

.primary {
  background-color: var(--md-sys-color-primary);
}
.primary-text {
  color: var(--md-sys-color-primary);
}

So I don't think is much interesting.

On the other side typography.module defines styles like

.display-large{
  font-family: var(--md-sys-typescale-display-large-font-family-name);
  font-style: var(--md-sys-typescale-display-large-font-family-style);
  font-weight: var(--md-sys-typescale-display-large-font-weight);
  font-size: var(--md-sys-typescale-display-large-font-size);
  letter-spacing: var(--md-sys-typescale-display-large-tracking);
  line-height: var(--md-sys-typescale-display-large-height);
  text-transform: var(--md-sys-typescale-display-large-text-transform);
  text-decoration: var(--md-sys-typescale-display-large-text-decoration);
}

for display-medium, display-small, and same for headline, body, label and title.

We can comment if this could add value to the existing Typography styles, but that could be a separated discussion / task.

@danice
Copy link
Author

danice commented Aug 28, 2023

BTW @bugy , checking the style definition for navbar, the M3 background color for the active element is not primary color, but md.sys.color.secondary-container.

So I propose adding this variable to _theme_variables
--secondary-container-color: var(--md-sys-color-secondary-container);

and this to _variables:
$secondary-container-color: var(--secondary-container-color) !default;

would this follow the framework style guidelines?

@wuda-io
Copy link
Member

wuda-io commented Aug 28, 2023

Yeah sure, I don't think there are any fixed guidelines yet, so we are open to make some. For my part, I like the approach from material design specs. So we do not have to reinvent the wheel with custom vars, but yeah i also like the interface approach with the var mapping. Lets move from here on. Sounds good to me 👍

@danice danice force-pushed the m3-styles branch 2 times, most recently from 1f96c0e to 5182e46 Compare August 29, 2023 12:38
@danice
Copy link
Author

danice commented Aug 29, 2023

After implemented the secondary-container-color for navBar as commented, the materialize web looked like this:

imagen

Is not bad, but personally I didn't like very much this tone for the main header. I have been doing some proofs with different colors, but this secondary-container-color is always a secondary-90 tone, always very light...

For this reason I decided to recover the colors.module file and add a .primary class to the navBar in the header.
With this it looks like:

imagen

For me it's much better Not really sure which I prefer, but looks more like the original website, and I changed the opinion about colors.module. I think we should include it as it's useful for this kind of adjustments.

@bugy
Copy link

bugy commented Aug 29, 2023

To be honest, I think we should avoid relying on classes coming from m3. Unless we explicitly want to say, that materializecss is m3 based. Rather than relying on a class, I would make navBar rely on another variable. So that when a user modifies corresponding CSS variable, he can expect that navbar color changes as well. However if we use .primary, navbar will not be changed, since it relies on --md-sys-color-primary and not on --primary-color

@danice
Copy link
Author

danice commented Aug 29, 2023

We are not forcing to use M3 classes. Using this classes is just a theme option and can be changed, just assigning _theme_variables.scss values to your preferences (exactly as before).

Adding this --secondary-container-color variable is needed to give te option to use that M3 color specification (and of course, the framework main page already states that it is following Googles Material guidelines).

The .primary class is just a class added to top navbar in materializecss website, it does not affect the components at all. And I don't pretend this to be the final look and feel of the website. It could be seen as an example of how to change the default component styles in some places (precisely trying not to be forced to use M3 style everywhere).

@wuda-io
Copy link
Member

wuda-io commented Aug 29, 2023

My suggestion would be to use the M3 Vars as a standard styling globally and for all components.
Later when we use web components, it would be cool to overwrite the styling per component.

Also I would exclude all the colors from the core library and make it as a seperate module or css file. Because I think in 80% of the use cases it is ok to just use primary and secondary and some warning and danger colors like defined in M3. If there are more colors needed then they can easily integrated.

Edit: I think in the long-term we should move in the direction of M3 Styling of the components because this is the overall mission of this framework. But it should stay independent and open. Also i think the guidelines from Google are very well organized and are used by many others too, so we should orient on them but not depend on them 👍

@wuda-io
Copy link
Member

wuda-io commented Sep 1, 2023

is this ready for a merge?

@danice
Copy link
Author

danice commented Sep 1, 2023

Yes it is. Now it includes fixes for the sidenav elements and it looks quite well

@wuda-io
Copy link
Member

wuda-io commented Sep 1, 2023

Ok awesome! Lets merge it and after that, seperate the Website step by step from the materialize core with the Vite-Proof.

Also:
There is a "test" directory in the core lib. Maybe we shoud make a testbed with all the components as seperate repo, but thats a whole other story. And more examples (maybe as own repo too) would also be cool.

@wuda-io
Copy link
Member

wuda-io commented Sep 1, 2023

Is this ok for the other members too? @bugy @mauromascarenhas

@wuda-io
Copy link
Member

wuda-io commented Sep 1, 2023

I am testing it atm, and there are still some issues with the sidenav.
grafik
and can you update the vars in the documentation too please? are they still the same?
grafik

@wuda-io
Copy link
Member

wuda-io commented Sep 1, 2023

Ok, its the dev branch. So lets merge it anyways.
I'll try to fix the smaller issues. Thanks for the contribution @danice !!

@wuda-io wuda-io merged commit 7442c8d into materializecss:v2-dev Sep 1, 2023
2 checks passed
@mauromascarenhas
Copy link

Guys, I'm sorry I was not able to check it before, but I was still reviewing the changes applied.

I have four points from what I could notice from the applied changes:

  • General opinion: Nice job! It was simply awesome (personaly, I really liked it)!
  • Colours: From what I could notice, those changes would not be a proper solution to [RFC] The state of material colors #54, since new "redundant/unused" variables are goind to be created as well. And, in addition to @wuda-io comment, I believe we are on the same page when mentioning that it'd be necessary to generate a separate CSS file for the "old style" colour classes, since it would avoid larger projects to break when migrating to Materialize 2.0 (a simple import would resolve the problem);
  • Fonts: I believe it would be a good idea to replace "fixed" font unities to relative ones ("pt"/"px" -> "rem"), since they would make the framework a little bit more accessible (they would respect the user preferences for font sizes 😄);
  • Final consideration: In fact, the docs should be upgraded ASAP so as to reflect those changes.

@quillaja
Copy link

So how do I use this if I'm not using SASS? I made a theme with that website and exported it to CSS. I extracted the "light.css" into my project. I added the light.css stylesheet to my page with the materialize components, expecting it all to magically change to my theme colors, but I'm still left with the default materialize colors (navbar has light blue background and white text....)

Some other website said something about adding a bunch of classes to my components, like .primary. That sounds incredibly tedious. Is that what I have to do?

@Jerit3787
Copy link

Jerit3787 commented Jun 20, 2024

So how do I use this if I'm not using SASS? I made a theme with that website and exported it to CSS. I extracted the "light.css" into my project. I added the light.css stylesheet to my page with the materialize components, expecting it all to magically change to my theme colors, but I'm still left with the default materialize colors (navbar has light blue background and white text....)

Some other website said something about adding a bunch of classes to my components, like .primary. That sounds incredibly tedious. Is that what I have to do?

you can import the light.css on the html you want, then set class .light on the body. It will reflect the whole ui. Then, if you want to change light or dark, import both dark.css and light.css and change the class between .light and .dark

@quillaja
Copy link

you can import the light.css on the html you want, then set class .light on the body. It will reflect the whole ui. Then, if you want to change light or dark, import both dark.css and light.css and change the class between .light and .dark

Thank you!

@Jerit3787
Copy link

Jerit3787 commented Jun 20, 2024

you can import the light.css on the html you want, then set class .light on the body. It will reflect the whole ui. Then, if you want to change light or dark, import both dark.css and light.css and change the class between .light and .dark

Thank you!

haha okay. i just realized there is a documentation on how to do it. https://materializeweb.com/themes.html. p/s need to update it later tho

@quillaja quillaja mentioned this pull request Jun 21, 2024
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.

6 participants