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

Please keep SASS optional #1332

Closed
jamesmfriedman opened this issue Sep 19, 2017 · 23 comments
Closed

Please keep SASS optional #1332

jamesmfriedman opened this issue Sep 19, 2017 · 23 comments
Assignees

Comments

@jamesmfriedman
Copy link
Contributor

Feature Requests

I keep up to date with changes to this lib, and have been loving using it. I was looking at the the most recent changelog and found that mdc-button--primary and accent were removed in 3e3c869 and now the suggestion for the breaking change is to use SASS mixins to replicate the behavior.

I'd would personally really love for SASS to remain something that is optional for this library. Looking at the code that was removed for the buttons, it's not something that could be easily replicated without SASS, but primary and accent buttons are used frequently in any design I've ever worked on.

In the end, this will allow people with all different types of css processors (or no processors) to get the most out of the lib.

Cheers!
James

@LeviHassel
Copy link

I second this. I'd love if there was a clean way to change the global primary and secondary colors without SAAS.

@jamesmfriedman
Copy link
Contributor Author

@LeviHassel there are using css vars. Just redeclare them on the :root element. Before this change, the buttons obeyed those vars which was excellent.

@LeviHassel
Copy link

Oh thanks! I'm new to CSS, so I wasn't aware of the :root element and have just been declaring the colors for everything on the html element. I still agree that the buttons should obey those vars though.

@lynnmercier
Copy link
Contributor

lynnmercier commented Sep 22, 2017

What if we added support for some custom CSS properties:

  • --mdc-button-ink-color
  • --mdc-button-container-fill-color
  • etc.

Then you could configure a text button to use your non-Sass theme like this:

--mdc-button-ink-color: var(--mdc-theme-secondary);

And your raised button like this:

--mdc-button-container-fill-color: var(--mdc-theme-secondary);
--mdc-button-ink-color: var(--mdc-theme-text-primary-on-secondary);

In fact you don't even have to use var(--mdc-theme-secondary)...you could use var(--tertiary-color-because-i-am-special). Material design felt that "primary" and "secondary" color theme slots might be too limiting. We wanted to open up customization of a button to work with a bigger theme palette.

Of course, IE/Edge don't really support custom CSS properties...so if you don't have Sass AND you want to support IE/Edge, we don't have an answer.

And, there are some Sass mixins which we cannot support with custom CSS properties. For example the mdc-button-filled-accessible requires if/else logic only possible in Sass, not runtime CSS. If any mixin's implementation changes such that we CANT support it with a custom CSS property...we would have to remove support of that custom CSS property.

Thoughts?

@mrijken
Copy link

mrijken commented Sep 22, 2017

+1

@Garbee
Copy link
Contributor

Garbee commented Sep 22, 2017

What was the reason for the removal in the first place? One of our initial goals was to make a full system capable of being used in sass as an option to get things optimized. Not a hard requirement to achieve basic well specified functionality.

@jamesmfriedman
Copy link
Contributor Author

The CSS var route is fine by me, but even with that it seems like the original support for primary and secondary should be library standard. I’m coming from this as a designer as well, I’ve never made an app in 12 years that doesn’t have some concept of at least primary. I appreciate the conversation on this!

@lynnmercier
Copy link
Contributor

We're updating the Button to inherit the primary color. We should have done this earlier, but looks like we missed it in our series of button pull requests.

#1356

So we will at least handle the concept of "primary" within a color palette.

@jamesmfriedman
Copy link
Contributor Author

jamesmfriedman commented Sep 26, 2017 via email

@acdvorak
Copy link
Contributor

IIRC, one of the main reasons we decided to remove the --secondary modifier class was because it was contributing significantly to CSS selector specificity hell. This is a consistent problem that we and our internal/external clients have run into over and over again, and it never seems to get any easier 😛 Our experience working on large projects like Google Search has taught us to simply avoid it as much as humanly possible.

If you only have one modifier class that overrides one property, it's quite manageable. But as soon as you have multiple states (enabled vs. disabled), multiple modifier classes that override multiple CSS properties (text vs. raised vs. stroked, which modify color vs. background-color vs. border-color), dark mode (which overrides some colors but not others), and custom client overrides via Sass mixins, the situation quickly spirals out of control.

Combinatorial explosion creates so many weird, hard-to-predict edge cases that writing bullet-proof CSS selectors that always do the Right Thing in every possible situation becomes very difficult, time consuming, and error-prone. It bloats our code and makes it harder to read, understand, maintain, and debug. It leads to subtle, hard-to-trace bugs for our clients, which wastes everyone's time.

In the interest of simplicity and robustness, it became clear that the least-crappy option was to nuke as many state combinations as possible. For button, this meant eliminating support for dark mode and --secondary.

In addition, our designers felt that the notion of a "secondary" button lacked a clear functional purpose and didn't have any semantic meaning. Whereas compact, dense, raised, and stroked buttons have specific design guidance around when to use each, a "secondary" button is purely aesthetic, which makes it hard to justify the additional code complexity and maintenance burden it requires.

As I'm sure you know, design and engineering often require balancing multiple competing interests and making imperfect choices that may not satisfy everyone. It sucks and we totally understand the frustration this might cause.

However, in our estimation, the benefits gained from removing --secondary and dark mode from button (namely simplicity, maintainability, and robustness) generally outweigh the costs (namely loss of convenience).

As @lynnjepsen mentioned above, you can override the CSS custom properties for your buttons, or if you need to support IE 11 💩, you can manually set the same style properties that our mixins use (though this will obviously be less resilient to any future changes in our internal implementation).

@jamesmfriedman
Copy link
Contributor Author

@acdvorak You have no idea how much I appreciate a clear and concise answer as to why it was removed. I've been working hard on a React Wrapper for this stuff and this seemed like a rather random breaking change, but I get it.

Thanks!

@djensen47
Copy link

Thanks for the detailed technical explanation.

However, if selector complexity is the issue, then isn't providing SASS mixins introduce the same technical issue?

Also, the docs here: https://material.io/components/web/catalog/buttons/ still reflect the old mechanism.

@acdvorak
Copy link
Contributor

@djensen47: The problem with the --secondary modifier class was that it needed to override a number of different color properties for every type of button. Since raised and stroked buttons require different ink (text) colors, --secondary needed to know about both types and do different things for each one. Combined with :disabled and dark mode, this is where selector specificity really became a huge headache, because you would end up with large compound selectors like .mdc-button--stroked.mdc-button--secondary.mdc-button--dark { color: ... }.

We partly solved this by removing dark mode and only allowing clients to customize the color of enabled buttons, but not disabled buttons. (We may revisit this in the future.) Our CSS selectors are now much simpler, which avoids a lot of specificity issues, and are specifically designed to avoid collisions between enabled and disabled states.

In addition, you now need to explicitly call mdc-button-ink-color(), mdc-button-container-fill-color(), etc. from within your own custom CSS selector instead of using a pre-baked --secondary modifier. If your selector has a classname or ID in it, the CSS generated by the mixins will have the same specificity as (or higher than, in the case of an ID) the base mdc-button classes. So as long as you include the mixins after importing the base MCW CSS, your selector should always take precedence.

The docs you linked to are updated now. We had some temporary issues pushing updates to the site, which is why it was previously out of date.

@djensen47
Copy link

Thanks for the explanation. 🥇 👍

@izakfilmalter
Copy link

izakfilmalter commented Jan 26, 2018

My big pain point with everything defaulting to primary is if you use the color tool, every action or clickable thing is actually using secondary which is also to spec with the the the color section of material.io.

So what ends up happening is that you almost have to swap primary and secondary to match the spec which is super annoying eg:

:root {
  --real-primary: #ffc107;
  --real-secondary: #1976d2;

  --mdc-theme-primary: var(--real-secondary);
  --mdc-theme-secondary: var(--real-primary);
}

This is the easiest way to get this lib to actually conform to the color guidelines which is really backwards.

Spec Examples

This is a frame from the video where we see that secondary is used for the highlight on tabbar and also for the fab.
image

Here we see that we shouldn't use primary colored actions on top of primary colored elements, yet this lib defaults to this behaviour.
image

Here we see that secondary should be used for TextField, but it defaults to primary.
image

Color Tool Examples

Here we see that the secondary color is used for the button instead of primary.
image

Here we can see that the secondary color is used as the default for the actions, eg button on card, and fab which is counter to the spec in this case.
image

@Garbee
Copy link
Contributor

Garbee commented Jan 26, 2018

Here we see that we shouldn't use primary colored actions on top of primary colored elements, yet this lib defaults to this behaviour.

If I recall correctly these things are changing in the future and that is why the coloring is the way it is. So MCW is actually ahead of the spec curve. In order to allow developers and designers more options on how they color things.

Here we see that secondary should be used for TextField, but it defaults to primary.

They can use, they are not required to use. Therefore using primary is just fine by default (and in fact as I recall from the spec text the recommended default.)

@izakfilmalter
Copy link

@Garbee If the case is that the lib is a head of the spec, then there are bigger problems than color. If you want to make a material app, you look at the spec to guide you, yet the official lib from Google deviates from it. It would be much better if the lib followed the spec, which in this case would mean updating the spec so that it is actually current with the direction of the design team and there for this lib.

@Garbee
Copy link
Contributor

Garbee commented Jan 26, 2018

I completely understand it is problematic from the outside looking in, but it isn't as easy as "just update it in sync". Especially when the spec revision specifics are still being worked out.

Also it doesn't make sense to build a reference implementation of the current spec when the team knows ahead massive changes are on the way which change critical implementation details. It's best to get ahead where it can be done in prep for the spec changes instead of using more resources going over the same stuff multiple times.

@lynnmercier
Copy link
Contributor

lynnmercier commented Jan 29, 2018

when the team knows ahead massive changes are on the way which change critical implementation details. It's best to get ahead where it can be done in prep for the spec changes instead of using more resources going over the same stuff multiple times.

Thats exactly right. We're trying to get ahead of the curve.

We're expanding our color system to be a bit more flexible than before—keep an eye out for these changes on our official Material guidelines.

We don't have a full timeline yet, but stay tuned by following the Material Design blog, or follow @materialdesign on Twitter.

@jamesmfriedman
Copy link
Contributor Author

On that point @lynnjepsen I had an idea for color handling that might be helpful.

Right now there are color variables like --mdc-theme-text-primary-on-background and --mdc-theme-text-primary-on-dark. I've been using these all the time to style my app in a way that will inherit changes to the variables.

I'd like to propose an additional option which is --mdc-theme-text-primary-on-context. There would be a version of this for all current variables, ie --mdc-theme-text-secondary-on-context. I think this would reduce the amount of overrides required for dark theme context switching and allow users to more easily opt in to dark mode. The variables could be declared like so.

:root {
 --mdc-theme-text-primary-on-background: black;
 --mdc-theme-text-primary-on-dark: white;

 --mdc-theme-text-primary-on-context: var( --mdc-theme-text-primary-on-background);
}

.mdc-theme--dark {
   --mdc-theme-text-primary-on-context: var( --mdc-theme-text-primary-on-dark);
}

I think this would greatly reduce inconsistencies in dark mode behavior, and if implemented in all of the core components, not much would have to be done to style them for the dark theme.

Happy to write up a larger proposal and take on the job of implementing it if you think it has any legs.

@JoeCodeswell
Copy link

+1 for material design components without sass

@erhard
Copy link

erhard commented Feb 7, 2019

+1 for without sass sass should be an option. Reason If you want to write web components you can style them without recompile them. sass is a precompiled . In case of web components I do not want to recompile every web component in order to run sass. The current system with --mdc-same-var and a default value is great.

@abhiomkar
Copy link
Collaborator

Closing this as obsolete. Please follow #5242 for more updates on this.

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

No branches or pull requests