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

Remove Classy and update to use starterkit #61

Open
occupant opened this issue Jan 9, 2023 · 16 comments
Open

Remove Classy and update to use starterkit #61

occupant opened this issue Jan 9, 2023 · 16 comments
Assignees

Comments

@occupant
Copy link
Member

occupant commented Jan 9, 2023

As per discussions with Joel, Darko, Classy is now deprecated, so we should move to use starterkit instead.

https://www.drupal.org/docs/core-modules-and-themes/core-themes/starterkit-theme

@occupant occupant self-assigned this Jan 9, 2023
@joelpittet
Copy link
Collaborator

@occupant should I create the PR against master or develop? I'm assuming master

@occupant
Copy link
Member Author

occupant commented Jan 9, 2023

Did a quick test locally and generally it was very simple.

  1. The classy/base library is excluded from Galactus, so this library and assets can be safely removed, but should we further investigate?
  2. When generating a new sub theme via php core/scripts/drupal generate-theme --starterkit galactus my_new_theme, there are several issues. Is this something we need to overcome?

Subtheme issues

  1. config namespace not changed (not unexpectedly)
  2. theme-settings.php function namespace not changed
  3. breakpoints settings key namespace not changed

@occupant
Copy link
Member Author

occupant commented Jan 9, 2023

@joelpittet Yes, I think master, at least for now, but we could use the opportunity to pull in some things from develop

@joelpittet
Copy link
Collaborator

joelpittet commented Jan 9, 2023

@occupant a bit of scope creep but the command you ran makes a theme based on galactus, we could ALSO recommend that and ditch the "UBC" theme example? AKA, no more extending base themes (though need an update path)

I plan on doing based on classy and copying the overrides back over the result in the PR

@occupant
Copy link
Member Author

occupant commented Jan 9, 2023

@joelpittet Oh, maybe I went about this wrong. I generated a new theme called Galactus first using php core/scripts/drupal generate-theme galactus. Then I copied in the Galactus assets (config directory, js, css, images, templates, etc) and updated the .info and template.php to consolidate them.

At that point everything seems to work just fine and we're decoupled from classy.

Looking back at that, I see I should have specified the galactus theme to be based off of classy, as you mentioned.

But next, I wanted to see what would happen if I then created a new theme (not subtheme - I misspoke above) based off of the updated Galactus. That's when I ran php core/scripts/drupal generate-theme --starterkit galactus galactusjr The subtheme [sic] issues are in galactusjr at that point.

Creating galactusjr was more a test to head off the inevitable request / question (new theme vs sub theme). But there do seem to be issues with that and I'm not sure if they're worth being concerned about.

@joelpittet
Copy link
Collaborator

@occupant I created the fork, not sure the namespace issues you mentioned above but maybe I should just do it too (for the experience) and then it might be obvious. I'll do that this afternoon (strike while the iron's hot) https://xkcd.com/356/

CC @darkodevubc In case you didn't/don't see this thread from your email already 😅

@occupant
Copy link
Member Author

@joelpittet Some more specifics about the issue I see when using the starterkit generated galactus theme to then generate an additional theme (galactusjr)

galactusjr.breakpoints.yml (lines 1, 7, 13, 19, 25, 31)

result

  • galactus.[breakpointname]
    expected
  • galactusjr.[breakpointname]

theme-settings.php (line 13)

result

  • function galactus_form_system_theme_settings_alter(&$form, FormStateInterface &$form_state) {
    expected
  • function galactusjr_form_system_theme_settings_alter(&$form, FormStateInterface &$form_state) {

config/install/

filename result

  • galactus.settings.yml
    filename expected
  • galactusjr.settings.yml

config/schema/

filename result

  • galactus.schema.yml
    filename expected
  • galactusjr.schema.yml

config/schema/galactus.schema.yml (line 3)

result

  • galactus.settings:
    expected
  • galactusjr.settings:

config/install/block.block.breadcrumbs.yml (line 9)

result

  • galactus
    expected
  • galactusjr

@occupant
Copy link
Member Author

Of course, the easiest and maybe best way to resolve the issues above are to add a note that Galactus can't be used to generate additional themes and should use a child theme

@occupant
Copy link
Member Author

@joelpittet As per message, Classy doesn't currently support being used as a starterkit (missing the required starterkit: true in the .info file).

I was able to get around this by adding it manually to Classy. I could then generate a new theme with php core/scripts/drupal generate-theme galactus --starterkit=classy.

I also, by way of comparison, generated a new theme using the defaults via php core/scripts/drupal generate-theme galactus (which seems to use stable9 as the starterkit if the base theme key in the .info file is an indication).

There are quite a few differences. I made a quick diff to show the files (left in attached image) from a default generated theme compared to (right in attached image) a theme generated with Classy as the starterkit. The key differences are additional .twig templates in Classy. I've included a screenshot and a diff of the changed files.
diff-galactus.txt
galactus-stable9-vs-galactus-classy

@occupant
Copy link
Member Author

@joelpittet I've taken an initial stab at it here:
master...feature/61-remove-classy-and-update-to-use-starterkit
We could remove the galactus/base (from classy/base) library and assets safely, but I've left them there for now

@occupant
Copy link
Member Author

occupant commented Jan 13, 2023

Looks like Galactus should have a version now in the .info. When I try to generate a theme using Galactus as a starterkit (which is allowed with the starterkit: true in the .info), you get a warning:

The source theme galactus does not have a version specified. This makes tracking changes in the source theme difficult. Are you sure you want to continue? (yes/no) [yes]:

Setting starterkit: false prevents Galactus from being used as a starterkit though and would be an acceptable solution.

@joelpittet
Copy link
Collaborator

joelpittet commented Jan 13, 2023

Looks like Galactus should have a version now in the .info.

I think a version is fine to add back, we should really try to version it in releases anyway I suppose

@joelpittet
Copy link
Collaborator

joelpittet commented Jan 13, 2023

I've taken an initial stab at it here: master...feature/61-remove-classy-and-update-to-use-starterkit We could remove the galactus/base (from classy/base) library and assets safely, but I've left them there for now

I hope to spend some time pouring over the diff later today, thanks @occupant

@darko-hrgovic
Copy link

darko-hrgovic commented Feb 3, 2023

Made a note on ubc-web-services/clf#27 that Galactus will be changing to starterkit and pondered how that will affect the CLF child theme.

I will also be creating a child theme from the CLF for each of our sites, so they'll likely need to be rebuilt using the CLF as starterkit.

That's a lot of template files stored in each theme (rather than previously inheriting from parent).

@joelpittet
Copy link
Collaborator

That's a lot of template files stored in each theme (rather than previously inheriting from parent).
@darko-hrgovic Yes, embrace the dark side of the fork!

@occupant We are seeing what you reported in #61 (comment)

@joelpittet
Copy link
Collaborator

#67 Fixes lots of the issues we ran into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants