google / material-design-lite

Material Design Components in HTML/CSS/JS
https://getmdl.io
Apache License 2.0
32.29k stars 5.03k forks source link

State classes don't align to BEM #1882

Open Garbee opened 9 years ago

Garbee commented 9 years ago

We have things like is-invalid, is-dirty, and is-disabled on textfield for example. This should instead be mdl-textfield--invalid, mdl-textfield--dirty, and mdl-textfield--disabled respectively for proper BEM structure.

This goes across any components where state classes exist.

Garbee commented 9 years ago

Quick note: Rather than maintain two sets of classes in 1.x and move to the new modifiers internally we need to do this in 2.x. It could be breaking if people's custom scripts are expecting is-invalid currently and we switch internally to the modifier.

posva commented 8 years ago

@Garbee is it that important to respect the BEM guidelines for classes that are never added by the user?

The current way is easier to interact with: you can write mixins that work across all components having those classes. I'll like to hear other opinions here. I do agree with the fact that they should be modifiers but do they need to?

Is there a problem with the code as it is right now? Is it harder to maintain or harder to understand (ex: less consistent). I didn't have any trouble when I started using mdl. My personal experience was to discover those classes while inspecting the code and it made me really happy being able to toggle the same classes for different web components.

Garbee commented 8 years ago

is it that important to respect the BEM guidelines for classes that are never added by the user?

We do uphold BEM. It shouldn't be a "We do, but only in X situations" kind of way. It will also make the new documentation structure we are looking at easier to understand what is going on. By breaking the CSS classes down by their alignment to BEM it is more clear to developers what is going on where. Having is-invalid under "Modifiers" is confusing, since it isn't aligning to BEM naming structure at all.

The problem is, we claim to uphold BEM structure. Are we really doing it? Right now, no we are not. So, it should get addressed.

As far as manual toggling, this is a red-herring to the primary point. And on consistency grounds, it is absolute inconsistent compared to what we claim to implement.

There is no real middle-ground here with BEM structure. It is either right or wrong, and our current implementation is wrong.

morlay commented 8 years ago

same as progress https://github.com/google/material-design-lite/blob/master/src/progress/_progress.scss

mdl-progress__indeterminate may be mdl-progress--indeterminate

posva commented 8 years ago

Indeed, I stepped into this yesterday.

Garbee commented 8 years ago

@morlay Good catch, but we can fix that one now...

sgomes commented 8 years ago

I disagree with moving away from state classes; we don't follow BEM absolutely strictly anyway, since we're adding a prefix to our components, mdl-. And we've got enough clout to define our own variant if we need to -- we can call it BEM-inspired if it upsets anyone :)

As a completely independent developer, following BEM strictly might not really come with any issues. But as a library, we need to clearly indicate which aspects of styling the library is in control of, and which aspects the developer is responsible for.

Having the is- syntax allows us to conveniently and easily distinguish between two very different concepts: transient states managed by the component itself (that is, auxiliary classes that help us with the styling for a particular state), and classes that the developer is supposed to set (that is, component options that fundamentally affect the way that component looks or behaves). That way, when the developer looks at the DOM and sees mdl-foo--bad-thing, they immediately know this is an option they set themselves, rather than something the component did on its own. Similarly, if they see is-bad-state, they know it's likely not their code at fault, but the component internals.

I'm happy for us to review the exact syntax we use, but I think we should definitely have two separate syntaxes for these two very distinct uses.

Garbee commented 8 years ago

Prefixes are allowed by BEM syntax, no problem with mdl- or whatever.

The is syntax for defining component actions is... a minor distinction that also is not really necessary. By doing that, we block ourselves from being able to easily use the BEM linter to verify our classnames meet BEM requirements. This means more cognitive work going forward with new components and a bigger area for code quality to lack in places. We've already had numerous alignment issues. A good syntax checker verifying our naming is needed for long-term maintenance of the project.

It doesn't really matter in what case something is modifying the component, all that matters is it is a modifier. Developers may end up writing these themselves (say to handle first-paint from server-side validation.) So the theory that is- means it isn't their fault is only true sometimes.

sgomes commented 8 years ago

Well, I definitely disagree that the distinction is minor :) Configuration and current state are two very different things, and it seems to me as inaccurate to group them together into a "modifiers" category as grouping them together into a "CSS classes" category.

If a user writes their own modifiers, they should do so in the scope of their own application, anyway. So, for example, .mdl-header.my-header.

Not being able to use BEM linters isn't enough of an argument, in my opinion. Ultimately, we shouldn't be doing BEM for BEM's sake; we should be using the tools that work for us, and changing or modifying them when they don't. And this is definitely one situation where I think grouping everything into one category would make things confusing.

@addyosmani @surma What are your thoughts on this?

dgrubelic commented 8 years ago

This reminds me of a discussion about the same issue @sgomes and i had few days ago. Same thing happened on the date picker (mdl-datepicker__date--selected vs. is-selected).

In my opinion, i think that is-selected is too abstract and very broad in definition when used globally in application. For example, someone can, by mistake, define some additional styling syntax which can interrupt internal component behaviour. I think that we should at least namespace is-selected by converting it to mdl-is-selected to protect it from that kind of situation. If someone is going to change "selected" state on date picker elements, he should be doing that intentionally (i know that this is corner case, but still it is possible).

Also, since is-selected can have different styling and behaviour in two different components, it should be in component namespace entirely.

Example, date picker can be opened on page load using is-visible modifier class. So that means that it is both application decision and user action that will determine component visibility state.

sgomes commented 8 years ago

@dgrubelic Thanks for chiming in!

I can definitely see the issue with being overly generic, yes. I could see us MDL-namespacing things, as you suggested (or even somehow moving things into the component namespace).

That said, I'd still like to see a syntactic distinction between state and configuration. Perhaps we should go the other way around and make configuration options stand out somehow? mdl-foo--config-bar?

KorsaR-ZN commented 8 years ago

@sgomes I fully agree with @Garbee I think you need to adhere to the general style of the whole project. And do not be divided into syntactic distinction between state and configuration.

sgomes commented 8 years ago

Alright, looks like I'm in the minority here, so maybe this isn't as much of a concern as I thought. Does everyone feel that documentation is enough to distinguish between state and configuration classes?

posva commented 8 years ago

What about mdl--is-selected ? adhering to a general style is a good practice but I really feel making a mdl-*--is-selected for each component makes state management harder to read when debugging and harder to code without bringing anything positive

jmichelsen commented 8 years ago

@dgrubelic in my opinion, the is-selected is only a state modifier, and an appropriately generic one that shouldn't have styles applied to it specifically, thus allowing it's use throughout a code base without much worry about different uses of it polluting styles. Specifically, when changing the state of the datepicker, for example. The styles for that change should live in .mdl-datepicker__date.is_selected in my opinion, not on the is_selected itself.

My two cents. State indicators should be generic and usable throughout the code base, regardless of the element they land on. So I'm more in line with you @sgomes, and agree with @posva on the debugging point. Maybe mdl--is-selected would be a happy enough medium.

KorsaR-ZN commented 8 years ago

@sgomes I think we should use the state as a global modifier .mdl--* as suggested by @posva. Then will simply be distinctions between state and configuration modifiers.

sgomes commented 8 years ago

Great idea, @posva!

.mdl--is-selected sounds good, and would follow BEM. @Garbee, WDYT?

Garbee commented 8 years ago

.mdl--is-selected is not following BEM at all. It is simply namespacing one issue away (which is good at least.)

It doesn't matter whether by user "configuration" or by the component doing something, the state of the given element is being modified. Therefore, direct modifiers should be used. Without creating our own tooling, this is the only way to use tools to check the classes automatically so we don't need to worry about doing it ourselves with every new component. Also it makes things very easily documentable unlike, "use X syntax in Y cases and B syntax in C cases."

dgrubelic commented 8 years ago

As i previously mentioned, just is-selected is too global. It should at least have some namespace prefixed to it. According to BEM, state or any other modifiers are to be attached to the block or element itself. If mdl-textfield is a block, his visibility modifier would be mdl-textfield--visible or mdl-textfield--is-visible. IMHO, if we are going to use BEM, then we use BEM as it is defined by specs. Additional and custom rules outside BEM can be confusing for new MDL developers.

I wonder what Harry Roberts would have to say about this one, since it seems to me that his naming convention we are using in MDL project. :)

Garbee commented 8 years ago

That is something else I'd like to change with 2.x, going to the actual BEM syntax (converting -- to _) so we don't need to manipulate the linter at all. Just tell it our prefix and off it goes.

posva commented 8 years ago

@dgrubelic Everyone can improve naming conventions :smile:

That generic state modifier or global modifier may exist when having a global prefix like mdl-, making state management easier. This modifier makes sense for properties such as is-visible and is-active and should not be added by the user when writing html, which is not the case for classic bem modifiers

dgrubelic commented 8 years ago

Agree, but i don't see any real benefit of improving BEM convention.

For that matter, i don't quite see any difference in writing:

// 1.
.mdl-textfield {
    &.is-selected {
        color: #ff0000;
    }
}

and...

// 2. 
.mdl-textfield {
    &--is-selected {
        color: #ff0000;
    }
}

..except, first example is nesting, and second is not. Also, first one has more weight than second. So if you would like to override first, you'd have to write something like:

.mdl-textfield {
    &.is-selected {
        &.my-textfield {
            color: #000;
        }
    }
}

..which would result in:

.mdl-textfield.is-selected.my-textfield {
    color: #000;
}

On the other hand, to override second example you can write something like this:

.my-textfield {
    color: #000;
}

More weight you have on selector, the harder you'll override it. For that reason BEM suggests avoiding nested declarations, global modifiers, etc...

posva commented 8 years ago

..which would result in:

.mdl-textfield.is-selected.my-textfield {
    color: #000;
}

You don't have a choice anyways because the is-selected is reused everywhere else

Agree, but i don't see any real benefit of improving BEM convention.

It's about making it easier to use developing features around it.

It looks to me that we're not looking at the modifier from the same perspective, I'm more concerned about writing code around it, which, in my opinion is the goal of modifiers like is-visible, is-active and so on, while you're more concerned about writing styles around or over it.

dgrubelic commented 8 years ago

Any example code of your concerns against mine?

You don't have a choice anyways because the is-selected is reused everywhere else

Actually i do. That's why we have v2 and no backward compatibility to stop us from defining stuff the way we want

It's about making it easier to use developing features around it.

Exactly. If i have to write .mdl-textfield.is-selected.my-textfield for every component or element inside component instead of .my-textfield, that is not very helpful. Makes app style harder to maintain and and override.

I think that there's no point of discussing about options here since we wrote some pretty convincing cases. It's just up to the arch team to decide what convention will be used.

posva commented 8 years ago

:neutral_face:

Any example code of your concerns against mine?

There is no against yours... Our points of view are different. It's like comparing apples to oranges

Actually i do. That's why we have v2 and no backward compatibility to stop us from defining stuff the way we want

What I mean is that if you want to redefine the style for your selected item you cannot write

.is-selected {
  color: black;
}

because it'll affect other stuff aswell. That's why I said there's no choice... It looked as if you took it personal... :disappointed:

Exactly. If i have to write .mdl-textfield.is-selected.my-textfield for every component

That just confirm what I was saying, we're not thinking about the same things, I'm talking about javascript interactions

I think that there's no point of discussing about options here since we wrote some pretty convincing cases.

:speak_no_evil:

It's just up to the arch team to decide what convention will be used

I'm not here to force people to take decisions based on my opinions. I'm challenging the choice of mdl-*--is-selected against a generic *is-selected I want to help out the arch team to take the right decisions, that's all :wink:

Garbee commented 8 years ago

If the guiding factor is a difference between what developers need to write and what the component does, then that is great because there isn't one.

is-selected, is-invalid, is-active, etc. These are all modes. Developers may write these to be served in order to prevent initial unstyled content when they know something is selected, or will be autofused, etc. So, they are simply pure modifiers.

Further, having secondary classes increases specificity. That is something we are hopefully going to work on reducing as much as possible in 2.x. Yet another reason to just do modifiers for everything and be done.

dgrubelic commented 8 years ago

Oh sorry, i didn't mean it like that. I was thinking about your point of view "against" mine, as in a way we can compare those two different opinions. No hard feelings here :)

I know you can't write

.is-selected {
    color: black;
}

That's what i'm saying. But you can write this without any problems and have lightweight selectors.

.mdl-textfield--selected {
    color: black;
}

Since we are clearly have two different viewpoints here, i would like to see code example for "javascript interactions" for both cases, global and component based. I think that could clear some things in our "little discussion" (before it turns to SPAM). :smile:

posva commented 8 years ago

If the guiding factor is a difference between what developers need to write and what the component does, then that is great because there isn't one.

Sorry, I don't understand it, can you please rephrase it @Garbee ?

is-selected, is-invalid, is-active, etc. These are all modes. Developers may write these to be served in order to prevent initial unstyled content when they know something is selected, or will be autofused, etc.

Yep, that may also happen through js

So, they are simply pure modifiers.

Well, that's the point, to me they're not bem modifiers like @sgomes said. At this point I just realised I missed 3 comments on the discussion, so I read them.

By doing that, we block ourselves from being able to easily use the BEM linter to verify our classnames meet BEM requirements. This means more cognitive work going forward with new components and a bigger area for code quality to lack in places. We've already had numerous alignment issues.

This looks quite important. Although I still think you can ignore the state/mode modifiers and use the linter. I don't want to be mean or anything here, but I feel some frustration in the sentence, maybe from an bad experience in the past with mdl. This may me think that the problem is bigger that just following BEM in order to be more structured. If having a linter and a syntax checker makes the development faster and the workflow smoother, the problem is not about following BEM. I may be completely wrong on this point but it is worth checking out

Further, having secondary classes increases specificity. That is something we are hopefully going to work on reducing as much as possible in 2.x. Yet another reason to just do modifiers for everything and be done.

That's neat. I've had trouble in the past redefining style because of this :stuck_out_tongue: This is also the point of @dgrubelic. Yet, modes are, to me, different. I agree with @sgomes on that.

@dgrubelic About the javascript example. I was thinking about mixins that toggle classes. About the .is-selected selector. I wasn't clear again :sweat_smile: What I meant is that if you decide to use .mdl--is-selected instead of .mdl-textfield--selected, You'll have to write in the styling the .mdl-textfield.mdl--is-selected selector and therefore if you want to overwrite it you'll have to use .mdl-textfield.some-special-class.mdl--is-selected anyways

dgrubelic commented 8 years ago

if you want to overwrite it you'll have to use .mdl-textfield.some-special-class.mdl--is-selected anyways

But that is the beauty of BEM modifiers (what @Garbee is saying from start we should use, and i'm supporting). With them you don't have to stack classes in order to override style.

You just use .mdl-textfield--selected {} and your done with it.

Garbee commented 8 years ago

This all comes down to reducing cognitive overhead. Treating things as "modifiers" or some other magical "configuration" parameter is a quirky line that muddys the way people need to think. It also can't be easily scripted to test against.

Either, we say we follow BEM or we remove it entirely. Since by doing this secondary class system we are the doing some partial-BEM and parital-something-else mix and even saying BEM is not useful since developers then expect it to be followed.

Overall, in my opinion for the best long-term maintainability of the project and for the simplest API to be given to developers, fully applying BEM is the way to go. It creates a very simple and easy to understand system.

posva commented 8 years ago

@dgrubelic And I also like it and find it beautiful :smile: but I find it confusing and less practical to mix up states/modes (logic related) with modifiers (style/dom related)

posva commented 8 years ago

@Garbee I understand your point and it's absolutely fine to fully apply BEM, yet I support @sgomes point of view about using the right tools. Specially when you say

Either, we say we follow BEM or we remove it entirely

That's sounds so extremist :stuck_out_tongue:

Thanks for taking the time to answer. Keep up the good work! :smile: ( :runner: :zzz: )

Garbee commented 8 years ago

It is extremest, yes. However that is what will lead to better long-term maintainability and a simpler contribution workflow. Instead of developers needing to think about what context a class is being used in, there is clear guidance as to how to name the structures.

sgomes commented 8 years ago

I disagree with one approach being clearly better over the other in terms of maintainability. We're defining our own convention, whether that means abandoning BEM, tweaking it, or strictly adhering to it, and I'm sure we'll still need to review every commit and educate contributors to the way we've chosen to do things. At the very least we need to ensure some sort of consistency and common vocabulary across components; we wouldn't want checked in one component, ticked in another, and on in a third, whether or not we're using component-specific classes.

I think the decision needs to come from considering all the upsides and downsides of each approach, so let's summarise the different options to see if we can arrive at a consensus.

.is-selected:

.mdl--is-selected:

.mdl-foo--selected:

.mdl-foo--is-selected:

Looking at these, I think the main decision is how much we want to emphasise cross-component consistency and coding. Given that we're moving to a more modular approach and a stronger API with v2, this is probably not the top priority; we can make sure that developers have all the information they need via the API, without having to rely on MDL state classes directly, which would at least take care of all the JS use cases. CSS cases are harder, but doable by having selectors with all the components (.mdl-foo1--is-selected, .mdl-foo2--is-selected, ...), and realistically we don't have that many components. The library development use-case isn't a problem. We could easily have mixins or generic code at multiple levels, as long as we define a constant inside each component as to what the component-specific class is (which we already do anyway with things like IS_SELECTED). That has me now leaning towards one of the component-specific approaches.

Looking at the two component-specific approaches, it looks to me like .mdl-foo--is-selected is the clear winner. We maintain all the benefits of the other one, and make it clearer what's configuration and what's state, simply by adding an extra 3 characters. We could consider this our own convention on top of BEM, and we can enforce it through the unavoidable review process.

As such, I'm currently leaning towards .mdl-foo--is-selected. Does anyone feel strongly against it after what I outlined above?

dgrubelic commented 8 years ago

This sounds like a compromise containing best of both concepts. Even tough option and configuration concepts still exist, i'd say that this is a good way to do it since we would fully endorse BEM standard.

I don't know if it matters, but this sounds like a good decision to me.

Garbee commented 8 years ago

.mdl-foo--is-selected is clearly the best choice if we keep some "configuration" vs "state" modification approach.

Given that we're moving to a more modular approach and a stronger API with v2, this is probably not the top priority; we can make sure that developers have all the information they need via the API, without having to rely on MDL state classes directly, which would at least take care of all the JS use cases.

Quite the contrary, this is one of the most important things with the styling we are working with. It is the API that developers will need to understand in order to work with the project and modify it. Further, relying on the JS API to be well documented doesn't matter. Developers are already trying to ignore the JS side and do things on the server to prevent FOUC. So that end of things shouldn't even be a big consideration here.

However, if we are to keep is-, in exactly which cases does this get used?

sgomes commented 8 years ago

Of course it matters, @dgrubelic! :) We're all contributors and/or users of MDL, so we should all get a say in things.

And thank you, everyone, for keeping a level-headed and open-minded discussion throughout the thread!

KorsaR-ZN commented 8 years ago

@sgomes .mdl-foo--is-selected - It sounds good, but I think the prefix is- does not quite fit What about what some abstract eg i- (internal)

.mdl-foo--i-selected .mdl-foo--i-active .mdl-foo--i-dirty .mdl-foo--i-invalid ...etc

This approach retains all the advantages, there is a clear distinction "configuration" vs "state" modification, and we follow the BEM :)

sgomes commented 8 years ago

However, if we are to keep is-, in exactly which cases does this get used?

is- classes are state:

Non-is- classes are configuration:

Does this make sense, @Garbee?

sgomes commented 8 years ago

@KorsaR-ZN Yup, i- would work for me too, I'm not too attached to is- :)

dgrubelic commented 8 years ago

I don't think that i- is good choice. It looks more like namespace rather then state indicator. With is-selected things are clear, but with i-selected developer might found himself wanting to know why (searching the docs) exactly is there i- before selected. I don't think that there is any need for unnecessary confusion here. Let the language itself speak the meaning. :)

Garbee commented 8 years ago

They are an internal implementation detail, not part of the component's API.

Well, they are a part of the API. We don't have a say in this. Developers will use this on their own to get styles at load time. So I don't see where this holds any weight.

Regular users shouldn't have to worry about these, only folks who really want to customise a component.

Because of the previous reasoning, users do worry about these. Invalid/selected/disabled (possibly more) are all states developers use at load time for FOUC prevention.

They are read-write to the component.

The only point that does hold weight to make a differentiation. However, where is the line drawn? Maybe at one point something is read-only, but later something changes and it then becomes read-write. Well, now we have class names that don't align or that we are stuck supporting two versions of. Overall, this just adds too much overhead to think about and makes more room for errors to happen (even if not intended.)

sgomes commented 8 years ago

Developers will use this on their own to get styles at load time. So I don't see where this holds any weight.

Well, just because folks do something doesn't mean we have to support it :) There is such a thing as wrong usage, and relying on internal implementation details is widely seen as a bad practice, across the entire industry.

The only point that does hold weight to make a differentiation. However, where is the line drawn? Maybe at one point something is read-only, but later something changes and it then becomes read-write. Well, now we have class names that don't align or that we are stuck supporting two versions of. Overall, this just adds too much overhead to think about and makes more room for errors to happen (even if not intended.)

Completely disagree, this is a matter of contract. If we're trying to be modular, we need separation of concerns. APIs can change, sure; but that's a change in contract, and will be reflected accordingly with a BC break / major version.

Ultimately, my point is that CSS classes shouldn't be a free-for-all. There should be things that serve as a way of communicating between component and developer, and certain things that the component can take onto itself to manage completely. And it should be clear which classes are which, since the web platform doesn't allow us to enforce these rules directly.

Garbee commented 8 years ago

So, we are fine telling people they shouldn't mark form fields as invalid on load and they instead should use the JS API on load to trigger that state?

sgomes commented 8 years ago

In my opinion, you're looking at that the wrong way; that's simply a use case that needs to be considered when redesigning the component. We've solved it by recommending people to use the internals in 1.x, but that's not necessarily the right solution.

If folks need a CSS solution for marking something as invalid on load, we should provide it, but it should be part of our contract.

We have private and public methods / properties in our JS, and it's also not possible to control access there. But we try our best to define a sensible API with everything that folks need marked as public, and the internals marked as private. We don't just give up and make everything public.

The same principle should apply to our CSS classes.

posva commented 8 years ago

.mdl-foo--is-selected :+1:

So, we are fine telling people they shouldn't mark form fields as invalid on load and they instead should use the JS API on load to trigger that state?

Sometimes classes just need to be toggled, I don't think enforcing the usage of the JS API is a good choice

Garbee commented 8 years ago

So how do we solve that use-case for textfields? Because that is the problem at hand and imo there isn't enough of a distinction between config vs state here to matter. It is super easy to just let people use the state classes as modifiers on load for the best performance. That is also the most natural way to handle the problem. Especially with us not being able to limit the exact markup structure that developers use, otherwise this entire thing is half-way pointless since we'd use native [invalid] and [checked] etc.

sgomes commented 8 years ago

@Garbee: The solution there, if we consider it to be enough of a use case, would be to have an option for marking things as invalid by default: mdl-foo--default-invalid, mdl-foo--invalid-by-default or something along those lines. It would be an option on the root node, so it wouldn't matter what the rest of the markup was (it makes sense to let the markup be open-ended with the restriction being that everything's a child of the root node, and that root node has the base class for the component).

Because that is the problem at hand and imo there isn't enough of a distinction between config vs state here to matter.

Even if we all agreed that was true for that particular case, it's a pretty big step to draw the conclusion that that is then the rule that should apply to the whole of the codebase. Again, if this were not an important distinction, we'd still all be using structs without caring about private vs public.

Reading the mood of the thread, I'm going to take a guess that the majority of folks seem to be leaning towards the .mdl-foo--is-selected approach. We seem to have arrived at what sounds like a reasonable compromise, and in the interest of reaching some sort of decision, I'm going to recommend we stick with that. If a significant number of folks strongly disagree, please add your comments, but otherwise let's assume that's the outcome for now.

KorsaR-ZN commented 8 years ago

let's choose .mdl-foo--is-selected :+1: :smile:

surma commented 8 years ago

I love the participation in this thread. Awesome work people!

After reading through it all, BEM-compliance (aka .mdl-foo--is-selected) gets my vote. Even though it is longer and will probably bump our total CSS size a bit, it will make user-side customization much easier (and we keep telling people to do that!!). Additionally, we get to ditch nesting SASS code, which has become a burden because our nesting tends to stretch across multiple screens. We might even get better specificity handling out of it. Linter is another big plus, although I suspect it will not work in the way we expect it to, because welcome to the internets.