Updated 'drupal/civictheme' to '1.13.0' and migrated the 'drevops' subtheme to SDC and npm tooling.#230
Merged
Merged
Conversation
📝 WalkthroughWalkthroughUpgrades ChangesCivicTheme 1.13 Upgrade and SDC Component Migration
Build Toolchain and Dev Dependencies Modernization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/themes/custom/drevops/components/04-templates/page/page.component.yml (1)
46-46: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate the description to reflect the new
noneenum value.Line 46's description states "Controls vertical spacing (top, bottom, or both)." but the enum now includes
'none'(Line 51). The description should be updated to reflect this. The downstreamdividercomponent already handles'none'safely via its conditional logic (falling through to an empty spacing class), and thelistcomponent already usesvertical_spacing: 'none'in test data, so the enum addition is functionally correct.📝 Proposed description fix
vertical_spacing: type: string title: Vertical spacing - description: Controls vertical spacing (top, bottom, or both). + description: Controls vertical spacing (top, bottom, both, or none). enum: - top - bottom - both - none🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/themes/custom/drevops/components/04-templates/page/page.component.yml` at line 46, Update the description for the vertical spacing property in the page component configuration to reflect the newly added 'none' enum value. The current description states "Controls vertical spacing (top, bottom, or both)" but the enum now includes 'none' as a valid option. Revise the description to include all four possible values: top, bottom, both, and none.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/themes/custom/drevops/templates/paragraphs/paragraph--civictheme-manual-list.html.twig`:
- Around line 43-51: The include statement passes the row_class parameter with
values 'row--equal-heights-content row--vertically-spaced' to both the spotlight
and grid components, but the spotlight component does not support or consume the
row_class parameter, causing these styling classes to be silently dropped when
layout equals 'spotlight'. Either remove the row_class parameter from the
include statement entirely, or conditionally pass it only when the layout is not
'spotlight' (by using a ternary or if condition to exclude row_class from the
spotlight include path while keeping it for the grid path).
---
Outside diff comments:
In `@web/themes/custom/drevops/components/04-templates/page/page.component.yml`:
- Line 46: Update the description for the vertical spacing property in the page
component configuration to reflect the newly added 'none' enum value. The
current description states "Controls vertical spacing (top, bottom, or both)"
but the enum now includes 'none' as a valid option. Revise the description to
include all four possible values: top, bottom, both, and none.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8eafa6bb-0293-489b-8230-57e6a1e87dbb
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
composer.jsonconfig/default/core.extension.ymlpatches.lock.jsonpatches/civictheme-remove-shortcut-mr19.diffweb/themes/custom/drevops/components/00-base/icon/icon.component.ymlweb/themes/custom/drevops/components/04-templates/page/page.component.ymlweb/themes/custom/drevops/drevops.info.ymlweb/themes/custom/drevops/templates/paragraphs/paragraph--civictheme-manual-list.html.twigweb/themes/custom/drevops/templates/paragraphs/paragraph--divider.html.twig
💤 Files with no reviewable changes (2)
- web/themes/custom/drevops/drevops.info.yml
- config/default/core.extension.yml
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.docker/cli.dockerfile:
- Around line 92-94: The cli.dockerfile has been updated to use npm ci for
package installation, but the GitHub workflow file still uses yarn install
--frozen-lockfile for frontend dependencies, causing a mismatch between local
container builds and CI runs. Locate the yarn install --frozen-lockfile command
in the GitHub workflow and replace it with npm ci to align with the npm-based
approach now used in the cli.dockerfile, ensuring consistent and reproducible
builds across both environments.
In `@web/themes/custom/drevops/.stylelintrc.json`:
- Around line 60-65: The regex pattern in the scss/dollar-variable-pattern rule
currently allows variable names like $rooted because the "root" alternation is
not anchored to require an exact match. Update the pattern to anchor the "root"
alternative so it matches only the literal "root" string and not names that
start with "root", while keeping the ct-* branch unchanged. This ensures the
pattern enforces the intended special-case exception for $root while
constraining other variables to follow the ct-* naming convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f0d6d161-9b98-414d-91ec-fbcb28a7c8e3
⛔ Files ignored due to path filters (2)
web/themes/custom/drevops/package-lock.jsonis excluded by!**/package-lock.jsonweb/themes/custom/drevops/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.docker/cli.dockerfileweb/themes/custom/drevops/.eslintignoreweb/themes/custom/drevops/.eslintrc.ymlweb/themes/custom/drevops/.storybook/main.jsweb/themes/custom/drevops/.storybook/manager.jsweb/themes/custom/drevops/.storybook/preview.jsweb/themes/custom/drevops/.storybook/sdc-plugin.jsweb/themes/custom/drevops/.storybook/theme.jsweb/themes/custom/drevops/.stylelintrc.jsonweb/themes/custom/drevops/components/00-base/spacing/spacing.scssweb/themes/custom/drevops/components/02-molecules/navigation-card/navigation-card.stories.jsweb/themes/custom/drevops/components/03-organisms/banner/banner.stories.data.jsweb/themes/custom/drevops/components/03-organisms/banner/banner.stories.jsweb/themes/custom/drevops/eslint.config.jsweb/themes/custom/drevops/includes/system_main_block.incweb/themes/custom/drevops/package.json
💤 Files with no reviewable changes (2)
- web/themes/custom/drevops/.eslintignore
- web/themes/custom/drevops/.eslintrc.yml
This comment has been minimized.
This comment has been minimized.
AlexSkrypnyk
changed the title
This comment has been minimized.
This comment has been minimized.
AlexSkrypnyk
added
the
Needs review
github-actions
Bot
added
CONFLICT
… switched build to npm.
AlexSkrypnyk
force-pushed
the
feature/update-civictheme
branch
from
317c7f0 to
2034ea9
Compare
github-actions
Bot
removed
the
CONFLICT
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@patches.lock.json`:
- Around line 2-9: The _hash field at the top level of patches.lock.json
contains an incorrect integrity value that does not match the actual file
contents. Update the _hash field value from the current incorrect hash to the
correct value provided in the comment to ensure composer-patches can properly
verify the lock file integrity.
In `@web/themes/custom/drevops/.storybook/preview.js`:
- Around line 144-150: The html.prettier configuration block in the preview.js
export is currently a top-level key, but the `@whitespace/storybook-addon-html`
addon expects this configuration to be nested under the parameters object at
parameters.html.prettier. Move the html object with its prettier configuration
(tabWidth, useTabs, htmlWhitespaceSensitivity) from the top-level export into
the parameters object so the addon can properly read and apply these Prettier
settings at runtime.
In `@web/themes/custom/drevops/drevops.theme`:
- Line 37: Before appending to $variables['attributes']['class'] at the line
containing $variables['attributes']['class'][] = $class;, ensure that the nested
array structure is properly initialized. Check if $variables['attributes']
exists and is an array, and if $variables['attributes']['class'] exists and is
an array, initializing them to empty arrays if they do not exist. This prevents
undefined-key warnings and ensures the append operation works reliably
regardless of whether drupal_attributes was previously defined.
In `@web/themes/custom/drevops/eslint.config.js`:
- Around line 40-42: The ESLint configuration in eslint.config.js is missing the
eslint-plugin-import plugin and the import/no-unresolved rule that are
referenced in the PR changes. To fix this, import the eslint-plugin-import
package at the top of the file, add it to the plugins object alongside
importNewlines with an appropriate key name, and then configure the
import/no-unresolved rule in the rules section to enable the rule so that
unresolved imports are properly checked instead of being ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1afaa023-90c8-4303-a029-5d25a1dc32c7
⛔ Files ignored due to path filters (3)
composer.lockis excluded by!**/*.lockweb/themes/custom/drevops/package-lock.jsonis excluded by!**/package-lock.jsonweb/themes/custom/drevops/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
.docker/cli.dockerfile.github/workflows/build-test-deploy.ymlcomposer.jsonconfig/default/core.extension.ymlpatches.lock.jsonpatches/civictheme-remove-shortcut-mr19.diffweb/themes/custom/drevops/.eslintignoreweb/themes/custom/drevops/.eslintrc.ymlweb/themes/custom/drevops/.storybook/main.jsweb/themes/custom/drevops/.storybook/manager.jsweb/themes/custom/drevops/.storybook/preview.jsweb/themes/custom/drevops/.storybook/sdc-plugin.jsweb/themes/custom/drevops/.storybook/theme.jsweb/themes/custom/drevops/.stylelintrc.jsonweb/themes/custom/drevops/components/00-base/icon/icon.component.ymlweb/themes/custom/drevops/components/00-base/spacing/spacing.scssweb/themes/custom/drevops/components/02-molecules/navigation-card/navigation-card.stories.jsweb/themes/custom/drevops/components/03-organisms/banner/banner.stories.data.jsweb/themes/custom/drevops/components/03-organisms/banner/banner.stories.jsweb/themes/custom/drevops/components/04-templates/page/page.component.ymlweb/themes/custom/drevops/drevops.info.ymlweb/themes/custom/drevops/drevops.themeweb/themes/custom/drevops/eslint.config.jsweb/themes/custom/drevops/includes/content_moderation.incweb/themes/custom/drevops/includes/system_main_block.incweb/themes/custom/drevops/package.jsonweb/themes/custom/drevops/templates/block/block--menu-block--civictheme-primary-navigation.html.twigweb/themes/custom/drevops/templates/block/block--menu-block--civictheme-secondary-navigation.html.twigweb/themes/custom/drevops/templates/paragraphs/paragraph--civictheme-manual-list.html.twigweb/themes/custom/drevops/templates/paragraphs/paragraph--divider.html.twig
💤 Files with no reviewable changes (4)
- web/themes/custom/drevops/drevops.info.yml
- web/themes/custom/drevops/.eslintignore
- web/themes/custom/drevops/.eslintrc.yml
- config/default/core.extension.yml
|
Code coverage (threshold: 80%) Per-class coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist before requesting a review
[#123] Verb in past tense.#123added to descriptionChangedsectionChanged
Updates
drupal/civicthemefrom 1.12.2 to 1.13.0. CivicTheme 1.13.0 completes the migration from the Components! module (drupal/components) to core Single Directory Components (SDC) and ships a new Storybook 10 / ESLint flat-config / Stylelint 17 frontend toolchain, so this PR brings thedrevopssubtheme in line on both the Drupal and the frontend side.Drupal and theme
composer.jsonconstraint~1.12.2->~1.13.0;composer.lockupdated.patches/civictheme-remove-shortcut-mr19.diffandpatches.lock.jsonre-rolled against 1.13.0's reorganised dependency block (the oldrest/taxonomycontext no longer matched; it now targetsserialization/system).paragraph--divider.html.twigandparagraph--civictheme-manual-list.html.twigmoved off the old@organisms/@baseComponents!-module namespaces to SDC component IDs (drevops:divider,drevops:list,civictheme:grid,drevops:spotlight), matching civictheme 1.13.0's own{% extends %}+{% include ... only %}pattern.components: namespaces:block indrevops.info.ymlis dropped (unused under SDC).page.component.ymladdsnoneto thevertical_spacingenum andicon.component.ymlmakessymbol/size/assets_dirnullable, so these subtheme components remain valid replacements of civictheme 1.13.0's relaxed schemas (otherwise SDC throwsIncompatibleComponentSchema, fatal on every page).core.extension.yml.drupal/componentsis added tocomposer.jsonso the deploy's config import can run the uninstall on environments where the module is still enabled; removing the package itself is a follow-up once the uninstall has propagated everywhere.drupal_attributesvia a shared_drevops_block_add_class()helper, because civictheme 1.13.0's block template renders that object rather than the standardattributes. This restores the two related Behat scenarios.templates/block/block--menu-block--civictheme-*-navigation.html.twig) to forward the preprocess-computedthemeto thecivictheme:navigationcomponent. CivicTheme 1.13.0's base templates include the navigation with{% include ... only %}and droptheme, so the navigation fell back to its light default and rendered low-contrast links on this site's dark header (components.header.theme: dark). Forwardingthememakes the navigation render with the dark theme again, matching the header.Frontend tooling
package.json,eslint.config.js,.stylelintrc.json,.storybook/*); the subtheme's custom rsyncbuild.jsis preserved. Storybook 8.6.7 could not build civictheme 1.13.0's components, which is what required this upgrade..docker/cli.dockerfile) and the theme lint step (CI workflow) use npm; the themeyarn.lockis removed andpackage-lock.jsonregenerated. The rootwebsitepackage (which lints custom-module JS/CSS) is a separate yarn package and is intentionally left unchanged.Out of scope
CivicTheme 1.13.0 post-update hooks introduce optional new config (a
civictheme_fast_fact_cardparagraph type, an eventhide_sidebarfield, etc.). These are not exported here - this site treatsconfig/defaultas authoritative and reverts untracked post-update additions, so adopting those features is a separate decision.Screenshots
Before / After