Skip to content

Conversation

@alan-cole
Copy link
Collaborator

@alan-cole alan-cole commented Nov 27, 2025

Drupal Ticket: https://www.drupal.org/project/civictheme/issues/3560225
UI Kit dependency PR: civictheme/uikit#840

Checklist before requesting a review

  • I have formatted the subject to include ticket number as Issue #123456 by drupal_org_username: Issue title
  • I have added a link to the issue tracker
  • I have provided information in Changed section about WHY something was done if this was not a normal implementation
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have run new and existing relevant tests locally with my changes, and they passed
  • I have provided screenshots, where applicable

Changed

  1. This will provide the table.twig component with headings that have been prepared for use in data-title="".
  2. It does check for a visually-hidden class, and if found, will not render any title. This is a simple implementation by design to avoid a few edge cases where visually-hidden elements are used in webforms,

Screenshots

Summary by CodeRabbit

  • Refactor
    • Improved table header processing to sanitize header text and correctly handle visually-hidden header content for more accurate screen-reader and visual output.
  • Chores
    • Updated theme UI toolkit dependency to a newer revision to include upstream fixes and improvements.

✏️ Tip: You can customize this high-level summary in your review settings.

…en prepared for use in data-title="".

It does check for a visually-hidden class, and if found, will not render any title. This is a simple implementation by design to avoid a few edge cases where visually-hidden elements are used in webforms,
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 27, 2025

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (2)
  • package-lock.json is excluded by !**/package-lock.json
  • web/themes/contrib/civictheme/package-lock.json is excluded by !**/package-lock.json

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Adds creation of a sanitized header array in civictheme_preprocess_table: each header is rendered to HTML, checked for visually-hidden classes, stored as an empty string if hidden, or stripped of HTML tags and stored in header_sanitized.

Changes

Cohort / File(s) Summary
Table header sanitization
web/themes/contrib/civictheme/includes/table.inc
Adds header_sanitized array: renders each header item to HTML, detects visually-hidden classes (visually-hidden, ct-visually-hidden), stores '' for hidden items, otherwise strips HTML tags and stores the result.
Dependency update
package.json, web/themes/contrib/civictheme/package.json
Updates @civictheme/uikit dependency commit hash from 73b0991... to 45c0254... in both root and theme package.json files.

Sequence Diagram(s)

(Skipped — changes are a focused preprocessing addition and a dependency bump; not suitable for a sequence diagram under the rules.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review the visually-hidden class detection logic and ensure it matches all theme variants (visually-hidden, ct-visually-hidden, possible variants).
  • Verify header_sanitized is populated after other header transformations and that any downstream consumers use the correct sanitized vs original header values.
  • Confirm no regressions introduced by the dependency hash change (build artifacts, lockfile differences).

Possibly related PRs

Suggested reviewers

  • cbiggins

Poem

🐰 Headers hop and softly hide,
I peek and strip the fluff inside,
Hidden bits I leave to rest,
Visible text I tidy best —
A tidy table, snug and spry.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main change: detecting visually-hidden classes in table headings to prevent markup breakage, which is the core objective of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@alan-cole alan-cole changed the title #3560225 Table headings with visually-hidden spans break table markup. Issue #3560225 by alan.cole: Table headings with visually-hidden spans break table markup. Nov 27, 2025
@alan-cole alan-cole added the State: Needs review Pull requests needs a review from assigned developers label Nov 27, 2025
@github-actions github-actions bot added State: CONFLICT and removed State: Needs review Pull requests needs a review from assigned developers labels Dec 9, 2025
@richardgaunt richardgaunt added the State: Ready to be merged Pull request is ready to be merged (assigned after testing is complete) label Dec 9, 2025
@richardgaunt richardgaunt merged commit 2443906 into develop Dec 10, 2025
15 checks passed
@richardgaunt richardgaunt deleted the feature/3560225-table-heading-visually-hidden branch December 10, 2025 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

State: Ready to be merged Pull request is ready to be merged (assigned after testing is complete)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants