592 move components to use new single directory components module #598
Conversation
Slightly changes the API to make it easier to use elsewhere without needing to rely on the subdirectory structure of localgov_base: icon_path in this version is a namespaced string representing the *complete* path to the directory that actually contains the required icons. Existing template left in place and probably will be even if we replace it in all localgov_base usage since we don't know what subthemes rely on it.
It's present in core now, but it's not automatically enabled. Since this branch requires it, it must be declared as a theme dependency.
Officially, these will be superseded by localgov:prev-next, though they'll presumably be retained for a time because we don't know which sub-themes/modules may have used them directly.
The Twig is largely identical to the original, with only a little regularization of variables, and use of the new localgov_base:icon SDC component. The CSS is unmodified.
To test, load LGD demo content from /births-deaths-marriages-and-citizenship/marriage-and-civil-partnerships/step-by-step/get-married-4
Test using LGD demo content /publications/publications-cover-page-demo-content/petitions-scheme-publications-demo-content
SDC enforces data types, so this is now unneeded.
Requires some variable renaming.
|
@markconroy here's my first attempt. The TL;DR is:
|
|
Thanks Christopher. For the "Icon" component, is there any issue with sites (sub themes) that currently use the icon template? I'm guessing they are using something like But now that will update to something like Does that mean we are going to break current sites? |
|
@markconroy That's why I left the original files in place with a big I could also update |
|
@petrillek pointed out to me that I've been rashly passing variables through twig 🙄. I'll correct that on Monday. |
|
This sh
8000
ould stay in draft status for the moment: there's an issue with variable data types in |
Code unchanged except to make text strings overrideable by calling templates.
Twig evidently treats TranslateableMarkup objects AS STRINGS, and bad things happen if SDCs declare them as objects.
The component is replaced *within* the theme by the localgov_base:prev-next SDC.
…ry-components-module
I've fixed that up, as well as converting the As with the other component, I've left the existing template and library files in place but added a |
|
Not sure what's going on in that failing test; seems unrelated. |
The component works without this, but only because _other_ code requires the same dependencies.
These files were moved into SDCs unmodified. This change removes them and updates the library definitions to use the files within the SDCs. We retain the deprecation notices on the library definitions since we'd like theme-users to use the SDCs.
The relevant library now uses the SDC component's js directly.
|
Discussing at Merge Tuesday - with @markconroy to review |
|
Thanks @markconroy. Been moving recently and lost the plot with these issues :) |
|
No worries @ctorgalson It's a bit of a large issue to get the head back into after not looking at it for a while. |
SDC functionality is now part of Drupal core - https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsolete#s-single-directory-components-sdc
|
@ctorgalson I think this is ready to merge. Do you want to give it a final test (especially as I have removed SDC as a dependency since it's now deprecated). |
|
@markconroy The standalone module is deprecated, but don't we need to declare it as a dependency to ensure the core module is enabled? |
…ry-components-module
|
I don't think so, it's now a service or something that in Drupal core, and not a module at all any more, so should "just work". I think it's this file: I removed it from our |
…ry-components-module
|
@markconroy we're a bit scared of this, it looks big! Waiting for your availability to discuss and liaise on the merge and release. Sounds really cool though! Thanks @ctorgalson |
|
It is big. It could open up a whole new paradigm for our frontend system. We are just testing it here with two components (icon and add-to-calendar) that we could easily revert if needed. Really, all that is happening is that the templates for those things are moving to somewhere else in the theme. In the future we can move more things, such as "Card view mode" so that if we wanted to have a card template for node and for user and for product and for paragraph etc, they could all extend from the same template. Pretty cool! |
|
@ctorgalson Perhaps you could come along to Tech Group Drop-in on Wednesday to help talk through what we are doing with this PR? |
|
@markconroy I can. Did it change from Thursday to Wednesday? (If so, can I get the Zoom link from you?) |
|
You can register here, so it will pop into your calendar for you - https://lu.ma/g5nsdt8b |
|
To test, install demo on default install and check event content type: https://localgov.ddev.site/events/sports-player-year-awards-2022 |
For the moment, this does the following:
drupal:SDCas a theme dependencylocalgov_base:prev-nextcomponent atcomponents/prev-next/localgov_base:iconcomponent atcomponents/icon/icon.html.twigwithlocalgov:iconin newlocalgov_base:prev-nextcomponent onlylocalgov_baseuses ofprev-next.html.twigwith the newlocalgov_base:prev-nextcomponentcss/components/prev-next.csstemplates/_components/prev-next.html.twigHow to test
Verify that no style or functional changes have crept-in. Mostly, this PR consists of moving twig templates and css, and updating other twig templates to reflect the new locations.
How can we measure success?
We can check that the expected markup/behaviour is present at content pages using each of the templates consuming the new component (obviously these changes don't exist on the live demo site):
templates/block/guides-prev-next-block.html.twig:templates/views/views-view-list--localgov-step-by-step-navigation--prev-next.html.twig:templates/navigation/book-navigation--publication.html.twig:templates/block/localgov-blogs-prev-next-block.html.twig:The changes here also anticipate that subthemes and other modules may be directly consuming the existing files and does not remove them.
Have we considered potential risks?
Images
The markup and CSS are identical to what's currently in production.
Accessibility
No changes of this kind have been made.