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

CSS coding standards: naming conventions #11689

Open
afercia opened this issue Nov 9, 2018 · 17 comments
Open

CSS coding standards: naming conventions #11689

afercia opened this issue Nov 9, 2018 · 17 comments
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Code Quality Issues or PRs that relate to code quality [Type] Discussion For issues that are high-level and not yet ready to implement.

Comments

@afercia
Copy link
Contributor

afercia commented Nov 9, 2018

The CSS naming convention used in Gutenberg doesn't meet the current WordPress CSS coding standards. Quoting from https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/

Similar to the WordPress PHP Coding Standards for file names, use lowercase and separate words with hyphens when naming selectors. Avoid camelcase and underscores.

Specifically, the underscore is not allowed. Either the WordPress CSS coding standards need to be changed (which is up to the 6 WordPress project leads) or the naming convention used in Gutenberg needs to be changed.

I'd like to note I've personally raised this issue a few times in the last months during the #core-editor team chats on Slack, but no action has been taken.

@afercia afercia added [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality labels Nov 9, 2018
@afercia
Copy link
Contributor Author

afercia commented Nov 9, 2018

@chrisvanpatten
Copy link
Member

One person's opinion: I think this is a case where the coding standards should be updated to allow underscores. Underscores in CSS class names are becoming incredibly common, through BEM and similar naming conventions, and I think only good can come from adapting these conventions. They make CSS class names and selectors more readable and help to better delineate their purpose and scope.

@chrisvanpatten
Copy link
Member

I'd also note that I interpret "Avoid camelcase and underscores" as a recommendation, not a hard requirement. To that extent, I think Gutenberg's use of underscores is technically permissible, even if it's not encouraged.

If the core standards aim to truly prohibit underscores, it should be phrased as "Do not use camelcase and underscores" so the intention is less ambiguous.

@afercia
Copy link
Contributor Author

afercia commented Nov 9, 2018

For clarity: I'm not opposed to update the current standards 🙂 I do strongly feel the mismatch need to be addressed before the release though. I'd say any software should be released meeting the standards the software established for itself.

If the core standards aim to truly prohibit underscores, it should be phrased as "Do not use camelcase and underscores" so the intention is less ambiguous.

Interpretation 🙂To my knowledge, core doesn't use underscores.

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Nov 9, 2018

Thanks @afercia. Re-reading my comments they may have come off a little rougher than I intended; sorry about that!

What would be the process for considering updating these standards? I know there are Core JS and PHP teams who tend to handle recommendations for those standards, but it's not clear what the process would be for CSS. The design team, perhaps…?

@afercia
Copy link
Contributor Author

afercia commented Nov 9, 2018

It does say "use lowercase and separate words with hyphens when naming selectors" and not "prefer", right?

@afercia
Copy link
Contributor Author

afercia commented Nov 9, 2018

What would be the process

To my knowledge, technical important decisions like coding standards should be made by the 6 project leads.

@chrisvanpatten
Copy link
Member

Fair enough. I know the JS team has collectively updated the JS standards during this process but I guess CSS doesn't technically have a team so it might be helpful to get some weigh-in from the leads on this (cc @pento? sorry…)

@afercia
Copy link
Contributor Author

afercia commented Nov 9, 2018

One more relevant point to clarify: as Gutenberg introduces this new "BEM-like" naming convention, does that mean that from the 5.0 release on this new convention can be used anywhere in WordPress?

  • If yes: I foresee problems in the consistency of the WordPress admin CSS
  • If no: is having a "double convention", one for legacy core and one for Gutenberg, desirable?

@afercia
Copy link
Contributor Author

afercia commented Nov 9, 2018

Fair enough. I know the JS team has collectively updated the JS standards during this process

Only partially :) There's the need to update other parts, but that's out of the scope of this issue.

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Nov 9, 2018

If yes: I foresee problems in the consistency of the WordPress admin CSS

I think this is inevitable in any kind of update/transition of the standards, but personally I think it would be great to see this updated across WordPress. CSS selectors in Core are often confusingly named, occasionally misspelled (.hndle, for instance), inconsistently applied (IDs in some places, classes in other places), etc.

We could continue to include those existing classes, but also append new/better classes and update core CSS to reference the newer/better selectors. That would be a great opportunity for improvement.

If no: is having a "double convention", one for legacy core and one for Gutenberg, desirable?

I don't know if I'd say desirable necessarily, but I don't think it's necessarily bad, either. Any code that would be directly merged into WordPress core SVN (instead of being pulled and built from npm, as it is now) should obviously use core standards, but for the code that will live primarily in GitHub I don't think it's a problem to use augmented/modified standards.

As long as the code here is consistent with itself, if it will be developed in a separate repository it shouldn't be too bad to use its own system.

@afercia
Copy link
Contributor Author

afercia commented Nov 10, 2018

@chrisvanpatten thanks for sharing 🙂Again, I've opened this issue not to discuss technical details. The main purpose here is outlining the need for documentation, and considering how to properly communicate changes to the standards (if any) to the developers community.

Let's put it this way: as a new contributor to core, the day after 5.0 is released I'll see there are 2 different CSS naming conventions. Am I allowed to use the BEM-like syntax outside of Gutenberg? Yes? No? Maybe? Confusion is great. This needs a decision and documentation.

Also, in #11677 (comment) @timelsass makes a good point:

there aren't really any publicly declared scss coding standards dictated for people to follow (at least none that I came across)

Gutenberg introduces an intensive usage of scss. Before Gutenberg, it was used only for the alternate color schemes in the admin. This reinforces the need for new documentation and updated coding standards. What are the rules to follow when writing scss? This needs to be documented and published on the Make core handbook.

@chrisvanpatten
Copy link
Member

With respect I think a bit of technical discussion is helpful to make the case for why changing the standards is beneficial (it is, in part, a decision that will have a technical impact) but I do appreciate that the next step here is getting a core developer/project lead to weigh in :)

@timelsass
Copy link
Contributor

I would love seeing the entire admin area take on this naming convention globally, and would definitely fit the scope of being a 5.0.0 major version bump as a lot of plugins do things with the admin css so they would need to be prepared. That feeling aside - WordPress code in EVERY language it uses ie PHP/JS/CSS is very un-uniform, and seems that each major feature added adopts it's own way of doing things -- despite the project already having clearly defined standards. Sometimes it's important to update to new standards as languages have progressed, but that should include tasking the time to refactor and maintain the old codebase instead of letting it become stagnant and unmanageable. In the very least having a roadmap and course of action to adopt the new standards in the existing codebase. Otherwise there's really no point in having any sort of publicly declared coding standards in WordPress as a whole because everyone just defines their own and either updates the standards by loosening the strictness of what is or isn't allowed. A naming convention discussion should have been one of the first discussions before code was even laid out, especially since it was the plan all along to introduce entirely new concepts. Now it's just an after thought, and makes it hard for new contributors to understand the codebase as a whole, especially with a lot of the backwards compat and individual coding styles that have made it up to be what it is today.

I agree with what @afercia has said, but also think @chrisvanpatten makes some good points too. Cross project standards DO make sense, and I do have many repos which contain different standards. It just becomes a tricky thing to balance out IMO when talking about the "WordPress Coding Standards" - as that to me means the all inclusive distributed package, WordPress. I will admit - I seldom have made commits, or even tickets on wp.org because svn and trac are not part of my standard workflow. I have no problem hopping in leaving comments in github - because I'm in here everyday for other thing :).
I think part of Gutenberg's increase productivity is not just from using more modern development practices (which clearly speed up development), but it has made contributing in various ways much more accessible to the larger development communities that have always existed outside of the wp.org ecosystem that don't generally want to spend their time going out of their normal workflows to discuss change or contribute to things. I think this was a major leap for WP.

I hope core adopts not only some of the naming conventions and coding styles implemented in Gutenberg globally, but also the workflow that makes it easier for more people to contribute to discussions, mockups, ideas, and general direction. All of these things are continuously complained about in the WP community, but continuing to support legacy ways can be harmful to productivity. I don't think it's that people don't want to contribute, help, or think Gutenberg is bad as some of the more controversial topics have been floating around - it's just more of the lack of accessibility to get connected with the information about it, and knowing where and how you can help without having to spend a week learning how to use svn, figuring out how to view code that's in trunk, understanding where in trac to go, how to reference issues, what to make tickets for, etc. I think at one point I did some theme reviews, and I honestly still don't have much of a clue how to go back to finding out which ones I gave feedback on. Perhaps it's just a culmination of these things that had prevented the planning and discussions on these sort of things to happen earlier on, but hopefully a standard for allowing cross-project standards becomes a thing while still having a roadmap to the future to have all projects eventually migrate to the new adopted standards set. It would make WP development much more enjoyable to sit down and view the entire application as a whole, and know that I could create a full fledged JS theme - 100% WP compliant, have the customizer settings globally for the site, and a wysiwyg editor all at my disposal to create more interactive interfaces and controls in and provide new immersive experiences for users.

I think the discussion needs to be way bigger - not just SCSS coding standards because it's much more than that which Gutenberg changes and sets as a standard.

Just a few major changes and decisions that Gutenberg really had to do:

  • SCSS Standards and naming conventions as discussed.
  • CSS Standards and naming conventions - after all the scss has to compile into something, and that's what most plugin/theme developers would be working with in overriding for their own functionality.
  • compiler standards - should output be bundled and minified in dist and src maps generated for developers only?
  • should a copy of the compiled src be available unminified and trigger via WP_SCRIPT_DEBUG
  • Modular structure standards
  • It introduces a new templating language via JSX.
    • for example SHOULD every property be listed on a new line and closing be on a new line?
    • Should a tag that has only one property open and close on the same line when it has no nested elements?
  • It introduces standards for interacting with reactjs and redux states - how or what are the methods core recommends? It seems for the most part - gutenberg has followed much of the react direction/philosophy, and all the leads have done a good job documenting most of this stuff - but I feel these need to be introduced as part of core development standards too.
  • This has also opened up using modern javascript, whereas the previous the standard was src just being written in a es2015 standard. A lot of the eslint standards defined !== the same as core JS standards.
  • Unit testing for JS being handled by Jest. It would make sense for the same testing infrastructure in the very least be shared between all projects.
  • Introducing webpack to the build chain instead of the grunt task runner doing everything. Now core has two ways of getting to the same end results in essence.
  • Dependency standards. One thing I noticed in gutenberg was preference given to lodash components (rightfully so), core should really push to move underscores out of the picture 100% and utilize lodash throughout the entire admin, no point in having both deps. I believe this may have been done or be in the works, but it does open up setting a standard for deps. It's very easy to integrate new controls, features, etc with a JS driven work flow, which usually makes development much more rapid. Having a leash and dependency review of new ones added should be part of a standard chain of command for merges/pushes in core code too. Obviously in this repo the leads have handled this well, and have some documentation on it, but it could use more depth when it comes to core deps now having more access to this workflow.
  • Prefer native vs lodash (?) - I prefer native over most any library, but importing a lodash method does save time in development.
  • JSDoc standards. In gutenberg this isn't reallllly handled 100% - but I feel as all new code submitted needs to ensure all params are documented correctly, and since versions applied. This may become confusing for people working and reading the code like plugin devs, as @SInCE 5.0 might be used for some core JS and/or functionality happening in the editor, whereas gutenberg would need to follow it's own versioning progression. This starts to provide some dissarray, but perhaps all projects need to start doing this, so the wp architecture becomes more modular and easily pulled apart instead of closely intertwined.
  • Class naming convertions in JS - multiple classes created in a single file? It becomes more convoluted with react as inheritance and OOP structure doesn't closely resemble what was the norm in core JS - but typically speaking gutenberg seems to follow one component or class per file - which is how I believe it's intended to be. However there are some things like this: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/color-picker/inputs.js Where inputs and input are both in the inputs.js - it is a little confusing structurally, but a standard should be set for this too.

There's plenty of other example in gutenberg of where it is changing the WP architecture, for the better, and hopefully core can adopt more of what it has done so far, and a better WordPress will come out of it all, SCSS coding standards and all!

PS sry 4.length

@mtias mtias added [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. and removed [Type] Bug An existing feature does not function as intended labels Nov 21, 2018
@talldan
Copy link
Contributor

talldan commented May 1, 2019

I think there's a fairly clear delineation in the standard that should be used when implementing CSS.

Was the design considered in a way that promotes reusable UI elements, and is the interface going to be implemented using a system that follows that methodology? (e.g. React or others)

  • If the answer is yes, then use the BEM style for the CSS, since BEM is really suited to this kind of methodology. It actively promotes writing CSS in a way that prevents cross-pollination between UI elements/components.

  • If the answer is no, and you're modifying something that's already in place then it's probably best to use the existing standard. This promotes consistency across the existing code base. It's incredibly hard to suddenly switch to something like BEM in an existing (and large) CSS code base. The idea of BEM is to prevent styles leaking/cascading into one another, but in an existing non-BEM code base you already have that problem and you end up having to write lots of resets for the legacy css (or attempting to namespace new css). I think you can only really consider using it when replacing something completely.

@afercia
Copy link
Contributor Author

afercia commented May 1, 2019

@talldan sure :) The original point of this issue is the following:

  • Gutenberg is part of core
  • Gutenberg introduces new CSS standards
  • as a consequence, the official CSS coding standards published at https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/ need to be updated
  • historically, updates to the coding standards required a formal proposal to be published on Make to collect feedback
  • we're waiting for the proposal and the CSS coding standards page to be updated since 2 years

Also, the following point still stands:

there aren't really any publicly declared scss coding standards (edit: or guidelines) dictated for people to follow (at least none that I came across)

@tellthemachines tellthemachines added the CSS Styling Related to editor and front end styles, CSS-specific issues. label Apr 23, 2020
@youknowriad youknowriad added [Type] Discussion For issues that are high-level and not yet ready to implement. and removed [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. labels Jun 4, 2020
@youknowriad
Copy link
Contributor

Looks like this issue might be something to consider for the #core-css meetings. cc @ryelle (update the docs in core about the CSS conventions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Code Quality Issues or PRs that relate to code quality [Type] Discussion For issues that are high-level and not yet ready to implement.
Projects
None yet
Development

No branches or pull requests

7 participants