Adds function to replace large class functionality in each component #78

evanmwillhite · 2017-05-05T20:13:42Z

Wouldn't it be lovely if instead of using different syntax for Drupal and Pattern Lab you could simply use this for adding classes <div{{ bem('block') }}> and it just magically worked?

And technically, the syntax is:

<div{{ bem('block', (block_modifiers), block_blockname) }}>

.. with the first being the blockname, the second being modifiers, and the third being a blockname override (modifiers and blockname optional). 💯

Changes:

Adds the bem function (components/_twig-components/functions/bem.function.php) to be able to pass in classes to Pattern Lab files (using simply class="") and Drupal files (using {{ attributes }}) correctly.
See components/_patterns/01-atoms/02-text/00-headings/heading-1/heading-1.twig and components/_patterns/02-molecules/_block.twig for an example usage of the new bem function. The block twig file is used directly in Drupal and correctly adds the bem class to the existing Drupal attributes

Testing instructions:

Install and run vanilla D8 site
Enable Emulsify as theme and checkout this branch (performing installation steps of course)
Download https://github.com/evanmwillhite/unified-twig-extensions/tree/develop and enable Unified Twig Extensions module.
npm start to run Pattern Lab and verify Header 1 is accurately printing classes from file and yml
Run Drupal site with preferred method and verify blocks are getting "block" class from components/_patterns/02-molecules/_block.twig as well as Drupal attributes.

To-do:

Get forked changes into original Unified Twig Extensions module
Add bem function usage in all base files
Update docs accordingly with second required Drupal module.

amazingrando

ModulesUnraveled · 2017-05-10T12:53:40Z

@evanmwillhite I'm assuming the answer is yes, but I want to clarify, can we also just do:

class="{{ classes('COMPONENT_NAME', COMPONENT_NAME_blockname) }}"

And, does the order matter? e.g. Could I do this:

class="{{ classes(COMPONENT_NAME_blockname, 'COMPONENT_NAME', COMPONENT_NAME_modifiers) }}

I also just noticed that you have COMPONENT_NAME in single quotes, but not the others. Are the quotes required?

evanmwillhite · 2017-05-10T13:27:13Z

We need to discuss this one before merging because of the ramifications on our Drupal work, but to answer your questions @ModulesUnraveled:

The first arg is currently required (base_class), so you couldn't leave it out although we could change that easily. This would be a string like "link" or "paragraph" - which is why it has quotes around it.
The order does matter, because it is the set order of the args being passed into the function.

rupl · 2017-05-26T15:36:27Z

I would propose you rename this function to bem() or something like that. Classes can and will be added by other sources so a more specific function name would be appreciated at least by me.

rupl · 2017-05-26T15:38:12Z

components/_twig-components/functions/blockname.function.php

+/**
+ * @file
+ * Add "classes" function for Pattern Lab.
+ */


add some params here? would help address @ModulesUnraveled's question regarding 'COMPONENT_NAME' (string) vs COMPONENT_MODIFIERS (array with no quotes)

evanmwillhite · 2017-06-08T17:33:57Z

As discussed in PG, let's see if we can:

only pass in a single class (although I thought we'd like to keep blockname and modifier as optional)
See if we can do this in such a way that Drupal doesn't need the Twig function.

rupl · 2017-06-13T16:07:29Z

components/_twig-components/functions/blockname.function.php

+  }
+  if ($blockname) {
+    $classes[] = $blockname . '__' . $base_class;
+  }


in the blockname conditional, I wonder if the modifiers loop should be repeated. It's a pattern I rely on frequently enough to standardize. Then you can target a modified component within a parent like so:

// abstract .blockname__baseclass--modifier {} // example .bios__profile--badge-webchef {}

rupl · 2017-06-16T11:34:23Z

I have implemented this in a project and the logic works as advertised. 👍

ccjjmartin · 2017-06-21T05:24:54Z

This is a great idea and something we need but there is a blocker for this to be compatible with Drupal:
https://www.drupal.org/node/2002606

We could create a sister Emulsify module to provide custom functions and filters until themes can provide their own. But right now this will appear to work within Pattern Lab but then getting it into Drupal will require setting up a custom module (granted it will be small). Example here if this needs to get into an existing client project:

https://api.drupal.org/api/drupal/core%21modules%21system%21tests%21modules%21twig_extension_test%21src%21TwigExtension%21TestExtension.php/class/TestExtension/8.2.x

https://drupal.stackexchange.com/questions/184184/error-when-creating-a-custom-twig-filter

rupl · 2017-06-21T07:51:10Z

Yeah there's precedent for having companion modules to themes. The original I know of is Omega Tools and there is also Magic which was intended as more of a shared module for several themes.

The PR linked after my previous comment is not visible to the public, but I started work in another 4K client repo and when it works we can use that as a starter. @ccjjmartin you pasted the 8.2.x link; a word of warning that 8.3.x is significantly different. Plus Drupal 8.4 was released yesterday. Just a heads up if someone decides to dive in.. don't use the old version! I got snagged on that while setting it up.

rupl · 2017-06-21T10:58:38Z

Something that had never occurred to me until I was fixing linting errors on this class I'm writing.. shouldn't the args be in a different order? It's called BEM but we're specifying EMB. Changing the order, plus the variable name baseclass to be element would probably smooth a lot of things over for someone who knows BEM but doesn't know Twig, Emulsify, etc.

evanmwillhite · 2017-06-21T14:45:44Z

OK, I have a lot to say here - sorry I realized as I was writing it out how long-winded it is. :) As far as an add-on module goes, I'm not crazy about the idea as it's yet another thing for Four Kitchens to support and it's currently a trivial amount of functionality to justify it (versus the modules listed above). If we could figure out a way to handle the classes correctly in PL and then deliver them as a string to Drupal attributes that would be ideal (only needing the Twig function on the PL side and therefore no Drupal module), but I can't think of how to do that off the top of my head.

If we do go the module route, we have a couple of options:

Help vet and release https://github.com/drupal-pattern-lab/unified-twig-extensions/tree/develop, which is a module by https://github.com/sghoweri that looks through all the Twig functions, filter and tags in the Pattern Lab directory and makes them available in Drupal. We might need to do some work here for Emulsify to make sure it can search our structure since we don't use the default _source/_twig-components Pattern Lab naming convention but rather components/_twig-components. This has the advantage of support outside the Emulsify community, but has the disadvantage of adding a lot of Twig stuff to Drupal that is unneeded (and/or duplicate).
Create our own module only for this single Twig function. I'll talk more about this in a second.

As for the BEM order, COMPONENT_NAME_blockname is currently just a way of overriding the blockname. So class="{{ classes('COMPONENT_NAME', COMPONENT_NAME_modifiers) }}" for a card component by itself might produce the following valid block and modifier syntax:

<div class="card card--large">

Here, "card" is the block name. When using COMPONENT_NAME_blockname (e.g., to add the following "grid__card" class), it actually currently tacks it on the end like so:

<div class="card card--large grid__card">

This is actually a non-standard BEM usage because there are now 2 blocknames (and 2 styles will be inherited). I think I personally would rather see us either:

get rid of COMPONENT_NAME_blockname as an option and call this what it is which is another modifier (see example below where you still want the "heading-2" styles but want it to look different in the card context):

<h2 class="heading-2 heading-2--card"></h2>

or 2. have the override replace the original like so (and use mixins, etc. to encapsulate any heading styles that you want to "inherit" in multiple places):

<h2 class="card__heading-2"></h2>

In writing this out, I actually think the 2nd option is what we need to do as it is true BEM and is therefore more readable/approachable while also allowing for the flexibility we like in being able to pass in a new block name.

In terms of where we go from here, I see the issue about handling Drupal attributes as being directly related to this PR. I think we should possibly rewriting this Twig function so that the following syntax (or something like it) would create the correct classes in both places:

<div {{ bem('card', card_modifiers, card_blockname) }}>

This would mean adding some functionality to the function to check whether Drupal attributes are in play and create this for PL:

<div class="card card--whatever">

and this for Drupal:

<div{{ attributes.addClass("card card--whatever") }}>

This above to me would justify the module, because it would solve both problems and be a really great addition to the wider Drupal/PL community. What do y'all think?

ccjjmartin · 2017-06-21T15:09:26Z

@evanmwillhite I was testing this last night in Drupal and ran into the issue that the Twig Parser class says the function doesn't exist. Example: we name the function bem(). When Drupal does the include on the twig file, bem() will fatal error (WSOD) with a message about bem() is not a declared function. So any non Drupal functions in Pattern Lab will have to be included within Drupal. If there is a module that searches Pattern lab and includes automatically (ideal solution) we should list that module as a dependency to unblock this and other twig functions.

evanmwillhite · 2017-06-21T15:29:33Z

Yea @ccjjmartin that's why this is not merged in yet - because it's currently a blocker for Drupal. If we can't write this as a function that only Pattern Lab would need (which I can't personally think of a way to do), I'd like to see us rewrite this function to print classes for PL and the correct attributes syntax for Drupal (see the last part of my comment above) and release both open-source (neither are Emulsify-specific).

rupl · 2017-06-22T12:30:04Z

whew! that is a lot. I have no strong opinions about the BEM really, the element thing was just a random thought I had. I also think a new module to supply the one twig function is overkill, maybe just some documentation or a nice detailed blog post would suffice for helping people implement in Drupal.

infiniteluke · 2017-07-13T18:51:41Z

That was fun the read, y'all! 💯

infiniteluke · 2017-07-13T19:53:01Z

package.json

@@ -8,6 +8,6 @@
  },
  "scripts": {
    "start": "gulp clean && gulp compile && gulp theme",
-    "postinstall": "rm -rf pattern-lab && composer create-project -n pattern-lab/edition-twig-standard pattern-lab && rm -rf pattern-lab/source && ln -s ../components pattern-lab/source"
+    "postinstall": "rm -rf pattern-lab && composer create-project -n pattern-lab/edition-twig-standard pattern-lab && rm -rf pattern-lab/source && ln -s ../components pattern-lab/source && git clone https://github.com/drupal-pattern-lab/bem-twig-extension.git && cp bem-twig-extension/bem.function.php components/_twig-components/functions && rm -rf bem-twig-extension/"


You may want to consider writing a small node script for this using https://www.npmjs.com/package/fs-extra and https://www.npmjs.com/package/cross-spawn so that emulsify will work on windows machines. If you want help with this, I can carve out some time!

This is a great idea - I created an issue over on emulsify-gulp and assigned you but feel free to move wherever fourkitchens/emulsify-gulp#34

ModulesUnraveled · 2017-07-18T22:00:58Z

components/_patterns/01-atoms/02-text/00-headings/h1/h1.twig

+#}
+{% set h1_base_class = h1_base_class|default('h1') %}
+
+<h1{{ bem(h1_base_class, (modifiers), blockname) }}>


So, one thing I found out using this on Ithaca, is that if we just use 'modifiers' at all levels, it gets passed down the chain. So, for example, if we set modifiers = [alt, light] to get class="h1-alt h1-light they'll also get passed down to the link component because it also uses the variable modifiers. So we'll end up with something like this:

<h1 class="h1-alt h1-light"> <a class="link-alt link-light></a> </h1>

(I've intentionally left out the href etc. to focus on the classes)

I didn't want that in my project, so I used the following in my link and h1 files respectively

<a{{ bem(link_base_class, (link_modifiers), link_blockname) }}>

and

+<h1{{ bem(heading_base_class, (heading_modifiers), heading_blockname) }}>

Yea, I wondered if I would have to do this. Done!

ModulesUnraveled · 2017-07-18T22:13:33Z

components/_patterns/00-base/layouts/grid/03-grid~divider-2.yml

@@ -1,6 +1,6 @@
 grid_label:
  "2-Column Divider Example"
-grid_modifiers:
+modifiers:


If you agree with my last comment about variable inheritance, these should be reverted.

ModulesUnraveled · 2017-07-18T22:14:31Z

components/_patterns/00-base/layouts/grid/_01-grid-item.scss

-// With Columns
-.grid__item {
-  .grid--columns-2 & {
+  @if $columns == 2 {


Interesting! I'll have to try to remember you can do this!

ModulesUnraveled · 2017-07-18T22:24:44Z

components/_patterns/01-atoms/01-links/link/link.twig


-<a class="{{ link_base_class_var }}{% for modifier in link_modifiers %} {{ link_base_class_var }}--{{ modifier }}{% endfor %}{% if link_blockname %} {{ link_blockname }}__{{ link_base_class_var }}{% endif %}" {% for attribute,value in link_attributes %}{{ attribute }}="{{ value }}"{% endfor %} href="{{ link_url }}">
+<a{{ bem(link_base_class, (modifiers), blockname) }}{% for attribute,value in link_attributes %} {{ attribute }}="{{ value }}"{% endfor %} href="{{ link_url }}">


All of these components can span multiple lines because of the way twig handles spaces. It makes it easier to read (classes on one line, attributes on another, href on a third) and tweaks are better represented in diffs since it's not all one line) Here's an example from one of my projects:

{% set link_base_class_var = link_base_class|default('link') %} <a {{ bem(link_base_class_var, (link_modifiers), link_blockname) }} {% for attribute,value in link_attributes %}{{ attribute }}="{{ value }}"{% endfor %} href="{{ link_url }}" > {% block link_content %} {{ link_content }} {% endblock %} </a>

Image is another good use-case:

<img {{ bem(link_base_class_var, (link_modifiers), link_blockname) }} src="{{ image_src }}" {% if image_alt %} alt="{{ image_alt }}" {% else %} aria-hidden="true" {% endif %} >

Link and image are good examples for this since they have multiple distinct parts. If a component only defines classes, I might not do it. e.g. this would look funny and be overkill IMO:

<p {{ bem(link_base_class_var, (link_modifiers), link_blockname) }} > {% block paragraph_content %} {{ paragraph_content }} {% endblock %} </p>

I'd just put the bem function inline with the
tag and save two lines.

I like this and have implemented it where it made sense

ModulesUnraveled · 2017-07-18T22:40:16Z

components/_patterns/01-atoms/02-text/text/01-paragraph.twig

@@ -11,9 +11,9 @@
 *   for example: A formatted text field in Drupal
 */
 #}
-{% set paragraph_base_class_var = paragraph_base_class|default('paragraph') %}
+{% set paragraph_base_class = paragraph_base_class|default('paragraph') %}


Does this work? Or does it need to be a separate variable?

Works great!

ModulesUnraveled · 2017-07-18T22:40:27Z

components/_patterns/01-atoms/02-text/text/02-blockquote.twig

@@ -12,9 +12,9 @@
 *   for example: A formatted text field in Drupal
 */
 #}
-{% set blockquote_base_class_var = blockquote_base_class|default('blockquote') %}
+{% set blockquote_base_class = blockquote_base_class|default('blockquote') %}


same question here

ModulesUnraveled · 2017-07-18T22:43:24Z

components/_patterns/01-atoms/03-lists/01-ol.twig

+<ol{{ bem(ol_base_class, (modifiers), blockname) }}>
+  {% for ol_item in ol_items %}
+    {% include "@atoms/03-lists/_list-item.twig"
+      with {


I don't know if we've talked about this, but I think we should standardize on having the with { not break to a new line. So this section would become:

{% include "@atoms/03-lists/_list-item.twig" with { "list_item_label": ol_item.label, "list_item_content": ol_item.content, } %}

Yea, I like this and I think it's becoming pretty standard. Done!

ModulesUnraveled · 2017-07-18T22:50:17Z

components/_patterns/01-atoms/06-buttons/02-button~alt-2.yml

@@ -1,7 +1,7 @@
 button_url:
-  "#"
+    "#"


four spaces

ModulesUnraveled · 2017-07-18T22:51:01Z

components/_patterns/01-atoms/06-buttons/button.yml

 button_content:
  "Default Button"
+button_url:
+    "#"


four spaces

ModulesUnraveled · 2017-07-18T22:51:13Z

components/_patterns/01-atoms/08-video/video.twig

@@ -14,9 +14,9 @@
 *   for example: to insert an html5 video component
 */
 #}
-{% set video_base_class_var = video_base_class|default('video') %}
+{% set video_base_class = video_base_class|default('video') %}


ModulesUnraveled · 2017-07-18T22:57:08Z

components/_twig-components/functions/bem.function.php

@@ -0,0 +1,42 @@
+<?php


Do we need to commit this file? Or will the install scripts thing put it here for us on npm install or yarn install?

I wasn't going to commit it, but once it gets added with the install script, it gets added to the repo. And I wasn't sure whether gitignoring it was a good idea either. What do you think?

Will we ever update it? And if so, do we want people to be aware of the update?

I'm thinking:
If we won't update it, git ignore it
If we will update it and git ignore it, they won't know there's an update
If we will update it and they commit it, they'll see the update when they do a git diff

Not sure what's right, just laying out the possibilities I see

well, we added it to the install process because we thought we might update it at some point, so... ?

ModulesUnraveled · 2017-07-18T23:02:25Z

templates/content/page-title.html.twig

@@ -17,7 +17,7 @@
 {% if title %}
  {% include "@atoms/02-text/00-headings/heading-1/heading-1.twig"
    with {
-      "heading": title,
+      "heading": title


We should keep the , on every line (including only lines, and last lines) so that PRs only show new lines of code and don't report this line as changed when we add a comma because we've added a second line.

Wow... that's kinda confusing... if we don't include the comma here, and then add a line to make it

"heading": title, "heading_blockname": "awesome",

The diff will show two lines have changed (because of the comma) instead of just showing the one new line as a change.

ModulesUnraveled

I mentioned a bunch of stuff, so requesting changes

Thins have changed since this was approved...

evanmwillhite · 2017-07-19T19:21:44Z

@ModulesUnraveled I've made a lot of changes per your review, and if I didn't I tried to address them in comments. Can you take another look when you get a minute? Thank you!

ModulesUnraveled · 2017-07-19T20:16:36Z

components/_patterns/01-atoms/02-text/text/02-blockquote.twig

-<blockquote{{ bem(blockquote_base_class, (modifiers), blockname) }}>
+<blockquote
+  {{ bem(blockquote_base_class, (blockquote_modifiers), blockquote_blockname) }}
+>


This is fine, but is a case where I'd probably just keep it inline with blockquote since it's the only attribute we're defining

ModulesUnraveled · 2017-07-19T20:26:14Z

components/_patterns/04-templates/example-page/example-page.twig

-  "heading": "This is an Example Page"
-}%}
+  "heading": "This is an Example Page",
+  "h1_modifiers": {


I don't know why I didn't mention this before, but this can be written shorter, like this:

"h1_modifiers": ["margin"],

And I'll give another example of multiples below

ModulesUnraveled · 2017-07-19T20:27:25Z

components/_patterns/04-templates/example-page/example-page.twig


 {% include "@organisms/card-grid/card-grid.twig" with {
+  "grid_blockname": "card",
+  "grid_modifiers": {


As promised, here's an example of how to write multiples in the array:

"grid_modifiers": ["divider", "columns-3"],

Fewer lines, easier to read.

ModulesUnraveled

One comment about where the example page file is placed. With that moved, this is as far as I'm concerned.

ModulesUnraveled · 2017-07-19T20:40:18Z

components/_patterns/04-templates/example-page/example-page.twig

@@ -0,0 +1,94 @@
+<div class="example-page">


I just realized that this file is in the 04-templates/example-page directory. I think it should be in the 05-pages directory (and not in a sub-directory) since it contains actual data, and not "replacement blocks".

ModulesUnraveled

Looks great!

added function to replace large class functionality in each component

9e862f8

evanmwillhite added the ready for review label May 5, 2017

removed publish dir

ca79489

amazingrando self-assigned this May 8, 2017

amazingrando added Status: Passes functional review Status: Passes code review and removed ready for review labels May 8, 2017

amazingrando previously approved these changes May 8, 2017

View reviewed changes

evanmwillhite mentioned this pull request May 26, 2017

Need to refactor all default components #48

Closed

rupl reviewed May 26, 2017

View reviewed changes

evanmwillhite added work in progress and removed Status: Passes code review Status: Passes functional review labels Jun 8, 2017

evanmwillhite assigned ModulesUnraveled and unassigned amazingrando Jun 8, 2017

rupl reviewed Jun 13, 2017

View reviewed changes

evanmwillhite assigned evanmwillhite and unassigned ModulesUnraveled Jun 21, 2017

removed local bem function and moved to postinstall script

d1a2171

infiniteluke reviewed Jul 13, 2017

View reviewed changes

Evan Willhite added 3 commits July 18, 2017 09:24

bem function in components

6ff8c77

Merge branch 'master' into class-function

c442996

major changes including bem functions across most components

dea2de6

ModulesUnraveled reviewed Jul 18, 2017

View reviewed changes

ModulesUnraveled added Status: In Progress and removed ready for review labels Jul 18, 2017

ModulesUnraveled suggested changes Jul 18, 2017

View reviewed changes

updates per review

d3d2a0c

Evan Willhite added 3 commits July 19, 2017 14:29

Drupal wysiwyg styles added

d0caad8

automated sprite spacing

7a80c32

documentation, commenting updates

3a778ad

ModulesUnraveled reviewed Jul 19, 2017

View reviewed changes

ModulesUnraveled added Status: Passes code review ready for review and removed Status: In Progress labels Jul 19, 2017

ModulesUnraveled approved these changes Jul 19, 2017

View reviewed changes

ModulesUnraveled added Status: Passes functional review and removed ready for review labels Jul 19, 2017

example page updates and array syntax fix

c41594a

ModulesUnraveled approved these changes Jul 19, 2017

View reviewed changes

evanmwillhite merged commit 5143f1d into master Jul 20, 2017

evanmwillhite deleted the class-function branch July 20, 2017 02:57

evanmwillhite mentioned this pull request Jul 20, 2017

Drupal attributes #86

Closed


		<a class="{{ link_base_class_var }}{% for modifier in link_modifiers %} {{ link_base_class_var }}--{{ modifier }}{% endfor %}{% if link_blockname %} {{ link_blockname }}__{{ link_base_class_var }}{% endif %}" {% for attribute,value in link_attributes %}{{ attribute }}="{{ value }}"{% endfor %} href="{{ link_url }}">
		<a{{ bem(link_base_class, (modifiers), blockname) }}{% for attribute,value in link_attributes %} {{ attribute }}="{{ value }}"{% endfor %} href="{{ link_url }}">

Adds function to replace large class functionality in each component #78

Adds function to replace large class functionality in each component #78

Conversation

evanmwillhite commented May 5, 2017 • edited Loading

amazingrando left a comment

Choose a reason for hiding this comment

ModulesUnraveled commented May 10, 2017

evanmwillhite commented May 10, 2017

rupl commented May 26, 2017

rupl May 26, 2017 • edited Loading

Choose a reason for hiding this comment

evanmwillhite commented Jun 8, 2017

Choose a reason for hiding this comment

rupl commented Jun 16, 2017

ccjjmartin commented Jun 21, 2017

rupl commented Jun 21, 2017 • edited Loading

rupl commented Jun 21, 2017

evanmwillhite commented Jun 21, 2017 • edited Loading

ccjjmartin commented Jun 21, 2017

evanmwillhite commented Jun 21, 2017

rupl commented Jun 22, 2017

infiniteluke commented Jul 13, 2017

infiniteluke Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ModulesUnraveled left a comment

Choose a reason for hiding this comment

evanmwillhite commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ModulesUnraveled left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ModulesUnraveled left a comment

Choose a reason for hiding this comment

evanmwillhite commented May 5, 2017 •

edited

Loading

rupl May 26, 2017 •

edited

Loading

rupl commented Jun 21, 2017 •

edited

Loading

evanmwillhite commented Jun 21, 2017 •

edited

Loading

infiniteluke Jul 13, 2017 •

edited

Loading