? Pending

User tests: Successful: Unsuccessful:

avatar ciar4n
ciar4n
12 Apr 2018

Pull Request for Issue #15279 .

Summary of Changes

Applies BEM class naming to top level elements in the login module. Initial PR with the intent to apply (if favorable) similar PRs to modules, components and layouts.

For these high level 'blocks' (https://en.bem.info/methodology/quick-start/) the intent is to use the Joomla architecture...

ExtensionType-Extension-View (Eg. com-content-archive).

avatar ciar4n ciar4n - open - 12 Apr 2018
avatar ciar4n ciar4n - change - 12 Apr 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 12 Apr 2018
Category Modules Front End
avatar brianteeman
brianteeman - comment - 12 Apr 2018

does bem require the double underscore?

avatar RonakParmar
RonakParmar - comment - 12 Apr 2018

Reference URL for BEM HTML class naming.
http://getbem.com/naming/

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 12 Apr 2018

@ciar4n can #15279 be closed?

avatar laoneo
laoneo - comment - 12 Apr 2018

@franz-wohlkoenig yes. If this one gets merged, then we know that the core will move to BEM.

avatar laoneo
laoneo - comment - 12 Apr 2018

@brianteeman yes, the element has two underscores and the modifier one, https://en.bem.info/methodology/naming-convention/.

96bd365 12 Apr 2018 avatar ciar4n CS
avatar ciar4n ciar4n - change - 12 Apr 2018
Labels Added: ?
avatar laoneo
laoneo - comment - 12 Apr 2018

What we do need to think about as well is the folder structure for the sass files of the blocks for easy reusability.

avatar ciar4n
ciar4n - comment - 12 Apr 2018

What we do need to think about as well is the folder structure for the sass files of the blocks for easy reusability.

Note: I suspect I need to stress that these particular classes are largely for identification rather than reusable OOCSS classes. That is why in BEM terms you will mostly see Blocks and Elements but no Modifiers in the changes. As we are using Bootstrap there is little or no need to be adding our own reusable modifiers. There may be cases where will will add modifier classes but I think best to add where needed rather than as a rule.

I am only focusing on top level blocks (eg. https://github.com/joomla/joomla-cms/blob/4.0-dev/modules/mod_login/tmpl/default.php#L27) and their subsequent children which I believe will cover the majority of identification required.

For block naming I was thinking along the lines of... ExtensionType-Extension-View. For default.php the view is left out. Template files (loadTemplate) I would consider Elements. Maybe someone has a better suggestion or can improve on this one?

Some examples...

mod-login - /modules/mod_login/tmpl/default.php
mod-login-logout - /modules/mod_login/tmpl/default_logout.php
com-content-article - /components/com_content/tmpl/article/default.php
com-content-article__links - /components/com_content/tmpl/article/default_links.php
com-content-category - /components/com_content/tmpl/category/default.php
com-content-category-blog - /components/com_content/tmpl/category/blog.php
joomla-pagination-links - /layouts/joomla/pagination/links.php
joomla-content-tags - /layouts/joomla/content/tags.php

Restate of goals..

  • A strict naming convention for HTML classes adoptable throughout Joomla.

  • All elements are uniquely identifiable and can be selected within CSS. This excludes framework classes (Bootstrap). This does not mean that every element needs to have class. In most cases elements can be selected by the class of a parent element.

  • The root element (containing div) of a view can be easily identified within the DOM.

  • By looking at the site source one can easily identify where an element is been rendered from within the Joomla architecture. Eg. It is easy to see from the following screenshot where the pagination rendered from (https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/pagination/list.php) ...

image

avatar laoneo
laoneo - comment - 12 Apr 2018

Would that have some influence on the custom elements?

avatar ciar4n
ciar4n - comment - 12 Apr 2018

Would that have some influence on the custom elements?

@dgrammatiko ??

avatar dgrammatiko
dgrammatiko - comment - 12 Apr 2018

@laoneo @ciar4n well no, or to be more precise here it shouldn't conflict. But let me elaborate a little bit here explaining some basic concepts.

  • Custom elements essentially are unique (dash separated) tags that in the HTML part can be the tag with any user defined attribute OR using the known W3C attributes (eg href, id, class). Also in the js part they are classes which extends the basic HTMLElement of the DOM.
  • Web components: basically are custom elements that encapsulate all their html in the shadow dom (or simply hidden from the normal DOM). This comes with some advantages eg you can have the same id in the DOM and in some element in the shadow dom without conflict, but also has some disadvantages: the css of the page will not be available in the shadow dom!

So for most of our components we will use just custom elements as we do want them to share the same css as the rest of the page (also the way we use forms prohibit us from using webcomponents for form elements or eg tabs that might contain form inputs). So whatever will be decided here custom elements will naturally support it and embrace it.!

PS For the few true web components BEM really makes 0 sense as any of the old class way makes very little sense to styling: eg: in the next image the styling is done in the <img> element itself and in the ::root which in this case is ttc-lazy-img (no need for classes, id's etc)
screen shot 2018-04-12 at 15 39 19

avatar ReLater
ReLater - comment - 12 Apr 2018

If this one gets merged, then we know that the core will move to BEM.

Who decides if this pr gets merged? I hope that we need more than just 2 successful tests for it.

mod-login-logout__login-greeting
Are you joking?

-1 for this pr. Who needs it? Just 5000 more BC breaks while upgrading from J3 to J4

avatar brianteeman
brianteeman - comment - 12 Apr 2018

i know that markup is not part of the b/c policy but we really should not be changing this just because. There has to be a good reason/benefit before this sort of change

my 5c (inflation and tax)

avatar dgrammatiko
dgrammatiko - comment - 12 Apr 2018

Just 5000 more BC breaks while upgrading from J3 to J4

You realise that you already have these breaks just by updating to BS4, right?

avatar brianteeman
brianteeman - comment - 12 Apr 2018

read it again Dmitris. these are joomla class names not bootstrap class names.

Just because semver says you can break things doesnt mean you should do without a very good reason. And I am asking what is that very good reason before I make up my own mind if this is a positive change or not.

avatar mbabker
mbabker - comment - 12 Apr 2018

So this is meant as a general comment as a whole and not directed toward any one person's work or effort.

Right now contributing to core and heavily the 4.0 work we mostly have a contributor pool who came onboard after 3.0's release in 2012 (some of you were probably using Joomla from 2.5 and before but weren't actively engaged here). Myself and Brian are struggling to count this, but aside from us two, @infograf768, and a couple others who've been longer term "come and go" contributors/maintainers, there aren't many involved in the project's teams right now who have also lead/managed work on a major version release. I point this fact out because while all of us would love to modernize all the things and make all the changes we would love to see happen and be able to use the "SemVer says we can break compatibility" excuse to justify it, the fact is Joomla is a distributed application with a vast user base and that means that those compatibility breaks MUST be well justified and documented if accepted (essentially as it relates to B/C you almost have to default to keeping the status quo and have a well constructed statement supporting changing it, that statement is going in part to be what convinces the code maintainers/owners the break is worth it and the communication end users see in why the break was made and how it is mitigated).

We've made mistakes in how our major version releases were put out in the past. We've learned lessons from that, either as end users consuming the software or the ones involved in the project working on those efforts. Use that knowledge and experience wisely when decision making.

avatar dgrammatiko
dgrammatiko - comment - 12 Apr 2018

And I am asking what is that very good reason before I make up my own mind if this is a positive change or not.

Well I am not the one to actually answer this, but to me this seems another valid proposal from @ciar4n that will eventually help the project:

  • to have more maintainable css code
  • to have more understandable/readable code
  • to standardise on some actual standards (more semantic), used by a great deal of devs, instead of us saying we're using BS and the rest is spaghetti code...

Sort term effect is the break of old CORE ONLY layouts
Long term effect (if this is followed correctly) a better css codebase.

But to put things in perspective here I'm neutral on this. I'm not a css expert so my point of view is just my point of view. But when someone who's actually expert in a field makes an effort to introduce another way before shutting them down we should very carefully listen and evaluate the actual proposal.
Also here I just provided the info that I've being asked (for CE) and made the obvious comment that layouts are already broken...

avatar mbabker
mbabker - comment - 12 Apr 2018

But when someone who's actually expert in a field makes an effort to introduce another way before shutting them down we should very carefully listen and evaluate the actual proposal.

Definitely not trying to shut it down. I think a lot of us "core contributors" grasp why the proposal exists and understand its benefits. But it's not just about ensuring you, or I, or any of the few dozen active folks on the project teams understand the logic behind it. It's also about ensuring that we can clearly communicate to end users why the change is being made; it's in part ensuring we don't make changes for the sake of making changes (just because we can doesn't mean we should) but making sound changes with a clear benefit.

avatar ciar4n
ciar4n - comment - 12 Apr 2018

mod-login-logout__login-greeting
Are you joking?

If we shy away from code based purely on its looks then I think we're often missing the point. 3 of the first 4 big brand sites I have just checked are all using BEM (http://www.bbc.com/, https://www.cnn.com & https://www.nationalgeographic.com/).

Just 5000 more BC breaks while upgrading from J3 to J4

I am failing to think of one? Maybe template providers targeting existing J3 classes in their CSS. It is hardly that big of a break and even they would appreciate a definite class naming convention. Currently you could expect anything. Take element root class names for example. Currently you could easily find any of the following...

  • mod-login
  • login
  • modLogin
  • mod_login
  • login-module
avatar brianteeman
brianteeman - comment - 12 Apr 2018

Maybe template providers targeting existing J3 classes in their CSS.

Exactly the point

It is hardly that big of a break and even they would appreciate a definite class naming convention

If they have to change all their templates for no gain then ...

And its easy for them but is it easy for the non specialist user

avatar mbabker
mbabker - comment - 12 Apr 2018

So on this PR's specific changes, out of context it is a minor B/C break by changing class names. No argument there.

To me, looking at things in the bigger picture, this would be my decision making process:

  • Is the HTML markup structure for the module still close enough to the existing Joomla 3 markup structure that existing CSS rules could still work without tweaks? If so, I'd suggest keeping the old class name as well as adding the BEM class name and adding deprecation comments (which we do have elsewhere in core, these are probably the hardest deprecations to track though).

  • Is the HTML markup structure for the module so vastly different that existing CSS rules wouldn't apply in any sane way? If so, then accepting this break on its own context doesn't really make things easier or harder and I'd say merge as is.

avatar ciar4n
ciar4n - comment - 13 Apr 2018

Is the HTML markup structure for the module still close enough to the existing Joomla 3 markup structure that existing CSS rules could still work without tweaks

In this case it is hard to say as the root element did not have a class. The structure has changed as one would expect with a BS2 to BS4 conversion but excluding that, not hugely. As the module is largely bootstrap elements, I suspect any use of the few non bootstrap classes would have been to target the inside bootstrap classes which will make such CSS now redundant anyways.

avatar Bakual
Bakual - comment - 13 Apr 2018

Mabe leave the old classes there and add the BEM ones? And with 5.0 clsan it up? No B/C issues this way.
You still have the ugly BEM classes of course.

avatar ciar4n
ciar4n - comment - 13 Apr 2018

Ok. I've added back in the old classes. I'm still of the opinion in most cases removing these classes would have minimal b/c effect. There is some obvious classes that would need to stay but I am failing to notice any in this PR. Certainly other changes have been made to the views that would cause similar minor b/c breaks and never seemed to be a problem.

If so, I'd suggest keeping the old class name as well as adding the BEM class name and adding deprecation comments (which we do have elsewhere in core, these are probably the hardest deprecations to track though).

How best to add deprecation comments? The only neutral way that I know of to mark actual classes visually is wrap them in square brackets...

<div class="mod-login__pretext [ pretext ]">

I don't mean to get peoples hair up here. I'm just looking to add a coding standard in an area where Joomla completely lacks one. BEM is currently the most common standard in this area. I have considered others but they either clash with their terminology (using words like component, module etc.) or would not cover all eventualities within Joomla. Yes it may be considered ugly and people suggesting this is a joke but I am not hearing any other suggestions.

avatar C-Lodder
C-Lodder - comment - 13 Apr 2018

Switching to BEM is all well and good, as it's a popular standard, however I'd suggest only doing it for classes that are being used. There's absolutely no point in keeping classes in views, if they're not being targeted via CSS or JS.

That said, I would say that adding the like of mod-login to the module wrapper element would be a nice "to have" so users can perform basic styling changes without having to create a template override.

So to sum it up:

Good:

<div class="mod-login">
    <input type="text" name="username">
    <input type="password" name="password">
</div>

Bad:

<div class="mod-login">
    <input type="text" name="username" class="mod-login__username">
    <input type="password" name="password" class="mod-login__password">
</div>

Same applies for other core extensions. Only use the likes of class="mod-login__password" if we're actually targeting it.

avatar ciar4n
ciar4n - comment - 13 Apr 2018

Switching to BEM is all well and good, as it's a popular standard, however I'd suggest only doing it for classes that are being used. There's absolutely no point in keeping classes in views, if they're not being targeted via CSS or JS.

I do agree however the issue here is how do we decide what classes would be used and what would not? Seems more logically to add them as a rule rather than having to ask that question with every class.

Also suggesting we only add classes to top level divs and their subsequent children (if they exist). Not each element...

<div class="mod-login">
    <div class="mod-login__form">
        <input type="text" name="username">
        <input type="password" name="password">
    </div>
    <div class="mod-login__whatever">
        <ul>
            <li>whatever</li>
            <li>whatever</li>
            <li>whatever</li>
        </ul>
    </div>
</div>
avatar C-Lodder
C-Lodder - comment - 13 Apr 2018

If you add them willy nilly, you're going to have a bloated DOM and a lot of unused classes. Remember that CSS is there for styling, not to be used an semantic identifiers.

Joomla core, is supposed to provide a core base for users to build on top of, allowing them to then extend it in any way they wish.

IMO it would be best to just stick to top level classes and of course continue to use the likes of mod-login__password, but only when being used. My 2 pence.

avatar ciar4n
ciar4n - comment - 13 Apr 2018

If you add them willy nilly, you're going to have a bloated DOM and a lot of unused classes.

I am not suggesting we do.

IMO it would be best to just stick to top level classes and of course continue to use the likes of mod-login__password, but only when being used.

How do you deem a class as been 'used'?

avatar C-Lodder
C-Lodder - comment - 13 Apr 2018
<div class="foo"></div>

Used 1:

.foo {
    background: #000;
}

Used 2:

var foo = document.querySelector('.foo')
avatar ciar4n
ciar4n - comment - 13 Apr 2018

Really? You are missing my point. Any class we add will potentially be 'used'. Add a class anywhere and someone somewhere is gonna use it. The whole point here is to do it in a way that has some kind of consistent reasoning. Just because we don't use a class with the default template doesn't mean there isn't good reason to have one.

avatar C-Lodder
C-Lodder - comment - 13 Apr 2018

Potentially, meaning by a user, or in core?

Don't get me wrong, I support the BEM approach, but adding classes for potential usage just reminds me of: #10011 when it should be up to the user if they want those classes or not.

avatar ciar4n
ciar4n - comment - 13 Apr 2018

Potentially, meaning by a user, or in core?

User

avatar ReLater
ReLater - comment - 13 Apr 2018

An example from real Joomla life:
In the last years many redundant blocks have been moved into JLayouts. What will happen with these rigid BEM classes in the future then. Changing it from mod-idontknow__whatever to joomla-forms-idontknow__whatever? Or has a pr like this to wait until Joomla 7 before it can be merged? Or an inconsistent mixture ? Or frustrating users that have to change these classes in their CSS files? Or?

At the end of the day BEM is unflexible in a flexible system like Joomla

avatar ciar4n
ciar4n - comment - 13 Apr 2018

Don't get me wrong, I support the BEM approach, but adding classes for potential usage just reminds me of: #10011 when it should be up to the user if they want those classes or not.

Obviously I don't agree. For me it is overkill to have to create an override just to add a class. I am not even suggesting we swarm the views with classes. Just enough that all elements are easily selectable and in a manner that follows some kind of standard.

avatar laoneo
laoneo - comment - 17 Apr 2018

@ReLater we have the same problem too without BEM. When we move some code into a layout which has some extension specific CSS classes then we do not want them in the layout. The idea is to start working in blocks, like (the class names of the blocks are just an example and hacked quickly together):

<div class="mod-login">
	<div class="mod-login__form">
		<div class="joomla-intro"><?php echo JText::_('INTRO'); ?></div>
		<div class="joomla-inputs joomla-inputs_horizontal">
			<input type="text" name="username">
			<input type="password" name="password">			
		</div>
	</div>
</div>

Using a concept like BEM actually forces the developer to think about the markup upfront.

avatar ciar4n
ciar4n - comment - 21 May 2018

It would be nice to get a decision on this due to its knock-on effect to any subsequent changes to the views?

avatar laoneo
laoneo - comment - 22 May 2018

I made very good experiences with BEM and on a long term it would be good for the devs to have a strategy for class naming.

avatar dgrammatiko
dgrammatiko - comment - 22 May 2018

I'm supporting this as well

avatar wilsonge wilsonge - change - 4 Jun 2018
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-06-04 21:31:44
Closed_By wilsonge
avatar wilsonge wilsonge - close - 4 Jun 2018
avatar wilsonge wilsonge - merge - 4 Jun 2018
avatar brianteeman
brianteeman - comment - 4 Jun 2018

It would be great if there was some documentation published on the docs site for this to aid developers

avatar ciar4n ciar4n - change - 11 Jun 2018
The description was changed
avatar ciar4n ciar4n - edited - 11 Jun 2018

Add a Comment

Login with GitHub to post a comment