feat: add build-theme command to sous #20

ccjjmartin · 2021-01-22T22:28:53Z

Purpose:

Allow devs to build the theme from the project root

Issues:

Closes: Add yarn build command #16

Notes:

This changes the name of the default theme from being the same name as the project directory to being a hard coded "emulsify", this makes sense to me but am curious to hear from others. I know we could have a naming collision with a module named "emulsify" but that seems avoidable.

americkson

@ccjjmartin Got a couple things here. We probably should discuss a bit and a just the approach here because I don't think we should have every project have the same theme custom/emulsify. There is a contrib/emulsify generated and the custom theme for the project should be based on the project or client name. Unless you were going for something else here?

americkson · 2021-01-31T00:39:17Z

sous/Starter.php

@@ -22,15 +22,15 @@ public static function installTheme() {
 $drupalFinder->locateRoot(getcwd());
 $composerRoot = str_replace('-', '_', strtolower(basename($drupalFinder->getComposerRoot())));
 // Execute the Emulsify theme build based on composer create path.
- shell_exec ("cd web/themes/contrib/emulsify-design-system/ && php emulsify.php $composerRoot");
- shell_exec ("cd web/themes/contrib/emulsify-design-system/ && npm install");
+ shell_exec ("cd web/themes/contrib/emulsify-design-system/ && php emulsify.php emulsify");


Making this change is taking away the custom theme name for the project. Not sure we want to go that route.

Background:
When this was originally setup the upstream theme was called emulsify but now we have two called:

emulsify-design-system
emulsify-drupal

So technically the emulsify name is now available.

Old logic problems:
As an example on the site I am working on now the directory was named texaslawhelp.org, so this script would have tried to create a theme called texaslawhelp.org

Brian ended up naming the theme on our project tlh_emulsify, but I am trying to see the benefit of naming every project we work on something different.

New logic problems:
I agree that picking a name for the end developer isn't an ideal situation though either.

Ideal world:
Ideally we would allow the developer to setting up the project to pick name the theme but that would require altering the rebuild.sh file with a different theme name for every project. Not a terrible ask but does require some additional work.

My recommendation would be that we change it back. That was/is one of the main points of this starter. To have a custom theme generated from Emulsify.

I see what you mean about that needing that consistency for the purpose of the shell build tool but I don't think that script should be the catalyst for this change. Maybe we consider this build-theme.sh file also gets generated on the fly. We can have a script that creates it fo us using the same method we are using to create the theme. OR we have some kind of documentation to address the need.

Also I found one more thing. There has been a recent change to the emulsify package name, which you mentioned. That has caused the contrib directory to change so this line should look like this:

shell_exec ("cd web/themes/contrib/emulsify-drupal/ && php emulsify.php $composerRoot");

I am thinking we should put this PR on hold because the emulsify.php script is going to be replaced with the new Emulsify CLI soon.

composer.json

americkson

@ccjjmartin Some thoughts on the direction here and requested changes.

americkson · 2021-02-04T23:26:55Z

.gitignore

@@ -116,3 +116,4 @@ example.sites.php
 default.services.yml
 default.settings.php
 settings.php
+web/themes/custom/emulsify


We don't want to ignore the generated theme. If this stays, then when someone pushed up the code to their own repo the custom theme will not be present.

This is in the section labeled # Remove me after sous install

americkson · 2021-02-04T23:28:17Z

sous/Starter.php

- shell_exec ("cd web/themes/contrib/emulsify-design-system/ && php emulsify.php $composerRoot");
- shell_exec ("cd web/themes/contrib/emulsify-design-system/ && npm install");
+ shell_exec ("cd web/themes/contrib/emulsify-design-system/ && php emulsify.php emulsify");
+ shell_exec ("cd web/themes/contrib/emulsify-design-system/ && yarn");


We should switch this back to using npm. We were getting some inconsistency with using yarn in the shell.

I also just noticed that we are running the npm install on the wrong theme OR we are missing this command for our custom theme... I'm not 100% sure if the base contrib emulsify-drupal theme needs it to run?

Either way to be safe we probably should add this line:

shell_exec ("cd web/themes/contrib/$composerRoot/ && npm install");

Brian was the one that shared some knowledge with me on yarn being what they use on Emulsify and he said they use it over there because the yarn.lock gives you exact packages as opposed to npm install which pulls the latest version of packages regardless of what is in the package.json, we switched to using npm ci on TLH which does install exactly what is in package-lock.json, but I do think we need to standardize across our projects if you are using yarn or npm ci. I opened a thread in the #engineers channel to discuss it.

I'm 100% on board for standardizing as much as possible. In this case we must use npm. The reason being is that we were having issues with the automated build when we had this set to using yarn.

I'll add some thoughts to the discussion in #engineers.

americkson · 2021-02-04T23:36:43Z

sous/Starter.php

@@ -22,15 +22,15 @@ public static function installTheme() {
 $drupalFinder->locateRoot(getcwd());
 $composerRoot = str_replace('-', '_', strtolower(basename($drupalFinder->getComposerRoot())));
 // Execute the Emulsify theme build based on composer create path.
- shell_exec ("cd web/themes/contrib/emulsify-design-system/ && php emulsify.php $composerRoot");
- shell_exec ("cd web/themes/contrib/emulsify-design-system/ && npm install");
+ shell_exec ("cd web/themes/contrib/emulsify-design-system/ && php emulsify.php emulsify");


My recommendation would be that we change it back. That was/is one of the main points of this starter. To have a custom theme generated from Emulsify.

I see what you mean about that needing that consistency for the purpose of the shell build tool but I don't think that script should be the catalyst for this change. Maybe we consider this build-theme.sh file also gets generated on the fly. We can have a script that creates it fo us using the same method we are using to create the theme. OR we have some kind of documentation to address the need.

Also I found one more thing. There has been a recent change to the emulsify package name, which you mentioned. That has caused the contrib directory to change so this line should look like this:

shell_exec ("cd web/themes/contrib/emulsify-drupal/ && php emulsify.php $composerRoot");

americkson · 2021-02-04T23:41:36Z

sous/Starter.php

 // Generate system.theme.yml and append new theme to install.
 $system_theme_yml = [
 "default" => $composerRoot,
 "admin"=> "thunder_admin"
 ];
 $yaml = Yaml::dump($system_theme_yml);
 file_put_contents('web/profiles/contrib/sous/config/install/system.theme.yml', $yaml);
- file_put_contents('web/profiles/contrib/sous/sous.info.yml', ' - '.$composerRoot.PHP_EOL, FILE_APPEND | LOCK_EX);
+ file_put_contents('web/profiles/contrib/sous/sous.info.yml', ' - emulsify'.PHP_EOL, FILE_APPEND | LOCK_EX);


Change this one back to the $composerRoot custom path.

callinmullaney · 2021-04-20T19:35:41Z

Per our conversation, this PR duplicates efforts with parts of the new Emulsify CLI. It should be closed. @ccjjmartin

americkson · 2021-04-20T20:57:27Z

Closing.

ccjjmartin added 2 commits January 22, 2021 16:27

feat: add build-theme command to sous

38f3701

fix: include build-theme in the rebuild command

39b8d35

ccjjmartin requested review from americkson and ModulesUnraveled January 29, 2021 16:52

ccjjmartin self-assigned this Jan 29, 2021

ccjjmartin added the 👍 Ready for Review Work is ready for review. label Jan 29, 2021

americkson requested changes Jan 31, 2021

View reviewed changes

americkson added invalid and removed 👍 Ready for Review Work is ready for review. labels Jan 31, 2021

ccjjmartin added 👍 Ready for Review Work is ready for review. and removed invalid labels Jan 31, 2021

fix: remove duplicate composer install script

d5cb78c

americkson requested changes Feb 4, 2021

View reviewed changes

americkson added 👉 Needs Work Reviewer has requested changes. and removed 👍 Ready for Review Work is ready for review. labels Feb 4, 2021

callinmullaney added duplicate This issue or pull request already exists and removed 👉 Needs Work Reviewer has requested changes. labels Apr 20, 2021

americkson closed this Apr 20, 2021

americkson deleted the build-theme branch April 20, 2021 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add build-theme command to sous #20

feat: add build-theme command to sous #20

ccjjmartin commented Jan 22, 2021 •

edited

Loading

americkson left a comment

americkson Jan 31, 2021

ccjjmartin Jan 31, 2021

americkson Feb 4, 2021

ccjjmartin Feb 5, 2021

americkson left a comment

americkson Feb 4, 2021

ccjjmartin Feb 5, 2021

americkson Feb 4, 2021

americkson Feb 4, 2021

ccjjmartin Feb 5, 2021

americkson Feb 5, 2021

americkson Feb 4, 2021

americkson Feb 4, 2021

callinmullaney commented Apr 20, 2021

americkson commented Apr 20, 2021

feat: add build-theme command to sous #20

feat: add build-theme command to sous #20

Conversation

ccjjmartin commented Jan 22, 2021 • edited Loading

Purpose:

Issues:

Notes:

americkson left a 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

americkson left a 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

callinmullaney commented Apr 20, 2021

americkson commented Apr 20, 2021

ccjjmartin commented Jan 22, 2021 •

edited

Loading