Migrate Batch 1 Components to SCSS modules

[andrewscfc]

Below is a summary generated by AI that I've edited. Some important context in there around specificity issues.

I'd like reviewers to take a look at the chromatic snapshot diff and see if you can work out an alternative explanation than a 'rendering artifact' for the diff.

Please note that the commit history is a bit messed up because the PR below this one was rebased and I merged it into this one. I tried using the stack feature but didn't realising merging a rebasing should not mix with that feature!

---

Summary

Batch 1 of the SCSS Modules migration converts Copyright, ReadTime, and InlineLink from Emotion to SCSS Modules.

EmbedError is also migrated because its consumer-owned Emotion override exposed a real styling regression in InlineLink.

The migration and styling guidance has been updated with the lessons from the implementation and review.

Changes

Component migrations

• Copyright

  • Migrated component styles to SCSS Modules.
  • Uses the element-qualified p.copyright selector to override the remaining Emotion colour rule from Text.

• ReadTime

  • Migrated component styles to SCSS Modules.
  • Preserves consumer-supplied className values with clsx.
  • Scopes the text-colour override to the existing .readTimeContainer context.

• InlineLink

  • Migrated default, visited, hover, and focus styles to SCSS Modules.
  • Uses one responsive rule set for typography.
  • Maps size and fontVariant through the shared typography helper into inline --gel-typography-* custom properties.
  • Avoids shipping a separate responsive CSS rule for every possible typography value.
  • Tests assert explicit custom-property values rather than repeating the production camelCase-to-kebab-case conversion logic.

• EmbedError

  • Migrated to SCSS Modules.

  • Its previous Emotion override changed the link's default colour, underline, and typography.

  • The override now uses the existing DOM context:

    .errorLinkWrapper {
      .inlineLink:not(:visited):not(:hover):not(:focus) {
        color: theme.$palette-black;
        border-bottom: #{theme.pixelsToRem-px-to-rem(1)} solid
          theme.$palette-black;
      }
    }

  • This changes only the default state. InlineLink retains ownership of visited, hover, and focus styles.

Consumer compatibility

• Audited all consumers of the migrated InlineLink.
• Removed the remaining css={styles.inlineLink} usage from Disclaimer.
• Kept Disclaimer's main migration out of scope because it still depends on the legacy GridItemLarge layout component.
• Moved Disclaimer's link override into its existing ancestor context and preserved its default, visited, hover, and focus states in index.styles.ts.
• Updated the three out-of-scope send/[id] pages to use a local copy of the former InlineLink Emotion helper in styles.ts.

Theme typography

• Added shared font variant aliases.
• Service font variant files now load the shared aliases.
• Static font variant mixins consume the theme-owned --gel-font-variant-* values.
• Fallback resolution remains in the theme layer rather than being duplicated in components.
• Added Sass compilation tests covering nested fallback chains and the final inherit fallback.

Guidance and documentation

Updated the component migration skill, webcore conversion skill, service theme skill, and styling standards to cover:

• Auditing shared-component consumers that pass Emotion styles through css={...}.
• Choosing data-* selectors for small sets of named variants.
• Using typed helpers and inline CSS custom properties for larger value sets.
• Using meaningful DOM context or element-qualified selectors for consumer overrides.
• Excluding child pseudo-states from consumer-owned default overrides.
• Minimising CSS for every site version (amp, lite and canonical)
• Measuring the final AMP style[amp-custom] payload and keeping it below AMP's hard 75 KB limit.
• The fact that Jest mocks SCSS Modules and does not validate the compiled stylesheet.

Useful Links

• Coding Standards
• Repository use guidelines
• CSS specificity and duplicated selectors - MDN

[Copilot]

🟡 Changes recommended

Unresolved moderate issues remain in EmbedError focus-class preservation and InlineLink’s 320–399px typography breakpoint.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/app/components/Copyright/index.module.scss:11

• This new module still hard-codes the ReithSans stack. The theme already centralizes that value as $reith-sans in src/app/components/ThemeProviderSCSSModules/fontFamilies.scss:34, and the migrated SCSS convention is to consume tokens through themeTokens; leaving the literal here can drift from the shared theme value. Forward the font-family token through themeTokens and use it here.

  font-family: ReithSans, Helvetica, Arial, sans-serif;

src/app/components/Embeds/EmbedError/index.tsx:24

• Passing styles.inlineLink replaces InlineLink's default className="focusIndicatorReducedWidth". Before this change EmbedError passed no className, so the link received the reduced focus ring; now it only receives the generic anchor focus rule, changing the component's focus treatment. Compose the contextual class with focusIndicatorReducedWidth instead of replacing it.

            className={styles.inlineLink}

src/app/components/InlineLink/index.module.scss:21

• This switches to the group-D custom properties only at 1008px, but the previous fontSizes styles and the SCSS gel-font-size mixin switch to group D at 600px (src/app/components/ThemeProvider/fontSizes.ts:15-18 and src/app/components/ThemeProviderSCSSModules/fontSizes.scss:236-240). Between 600px and 1007px, migrated links therefore keep the smaller group-B typography. Use the font-specific group-D breakpoint instead.

  @media #{theme.$mediaQueries-group-4-min-width} {
    font-size: var(--gel-typography-font-size-group-d, inherit);
    line-height: var(--gel-typography-line-height-group-d, inherit);

src/app/components/ReadTime/index.tsx:94

• This change specifically promises to preserve consumer-supplied className values, but the ReadTime suite never renders one. Add a test asserting that a supplied class is present alongside styles.readTimeContainer, so a future regression to overwriting or dropping the consumer class is caught.

      className={clsx(styles.readTimeContainer, className)}

• Files reviewed: 48/48 changed files
• Comments generated: 2
• Review effort level: Lite

[andrewscfc]

Addressed through moving test coverage to to helper rather than exhaustively testing it in lnline Link: 668b4bf

[Copilot]

🔵 Needs a closer look

Two moderate issues remain involving EmbedError focus styling and serifRegular fallback behavior.

Review details

Suppressed comments (2)

src/app/components/Embeds/EmbedError/index.tsx:24

• Passing styles.inlineLink replaces InlineLink's default focusIndicatorReducedWidth class instead of adding to it. Embed errors therefore lose the reduced-width focus treatment and fall back to the generic anchor focus rule; combine the two classes here (or make InlineLink merge its default class with consumer classes).

            className={styles.inlineLink}

src/app/components/ThemeProviderSCSSModules/fontVariants/fontVariantAliases.scss:54

• This alias changes the existing serifRegular behaviour for services that define --serif-regular-*. The legacy resolver deliberately maps serifRegular to the serif medium variant (src/app/components/ThemeProvider/fontVariants/index.tsx:24-25), and the existing Text test expects ReithSerif weight 500 (src/app/components/Text/index.test.tsx:32-44), but this chain resolves --serif-regular-font-weight first, which is 400 in fontVariants/reith.scss. Make the serif-regular alias start at serif-medium (while retaining the sans fallback) and add a regression assertion for this chain.

  @include define-font-variant(
    'serif-regular',
    'serif-medium',
    'sans-regular'
  );

• Files reviewed: 51/51 changed files
• Comments generated: 0 new
• Review effort level: Lite

[andrewscfc]

src/app/components/Embeds/EmbedError/index.tsx:24

* Passing `styles.inlineLink` replaces InlineLink's default `focusIndicatorReducedWidth` class instead of adding to it. Embed errors therefore lose the reduced-width focus treatment and fall back to the generic anchor focus rule; combine the two classes here (or make InlineLink merge its default class with consumer classes).

            className={styles.inlineLink}

Addressed in 66afced

src/app/components/ThemeProviderSCSSModules/fontVariants/fontVariantAliases.scss:54

* This alias changes the existing `serifRegular` behaviour for services that define `--serif-regular-*`. The legacy resolver deliberately maps `serifRegular` to the serif medium variant (`src/app/components/ThemeProvider/fontVariants/index.tsx:24-25`), and the existing Text test expects ReithSerif weight 500 (`src/app/components/Text/index.test.tsx:32-44`), but this chain resolves `--serif-regular-font-weight` first, which is 400 in `fontVariants/reith.scss`. Make the `serif-regular` alias start at `serif-medium` (while retaining the sans fallback) and add a regression assertion for this chain.

  @include define-font-variant(
    'serif-regular',
    'serif-medium',
    'sans-regular'
  );

This is a valid observation: the legacy Emotion resolver currently maps serifRegular to serif.medium, which gives ReithSerif weight 500, while the SCSS alias prefers the explicit serif-regular token, which gives weight 400.

The SCSS behaviour is intentional and was introduced earlier in f0bc46f when the serifRegular font was corrected to reflect what must have been originally intended.

I'll address the inconsistency in a follow-up PR by updating the Emotion resolver and its consumers to use the same font fallback chain.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Responsive typography B/D properties lack explicit compiled-value coverage, and the new declaration still needs its required semicolon.

Review effort: Lite
Findings: 1

Open (1)

• getTypographyStyles now emits font-family, font-style, and font-weight custom properties for each…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Two moderate findings remain unresolved.

Review effort: Lite
Findings: 1

Open (1)

• getTypographyStyles now emits font-family, font-style, and font-weight custom properties for each…

Previously missed (1)

In code that hasn't changed since last review

Add assertions for group B/D size and line-height mappings

src/​app/​components/​ThemeProviderSCSSModules/​typography.ts:31

The size branch now emits group-B and group-D font-size/line-height custom properties, but the test table only asserts group A. The Sass test confirms that the mixin references B/D variables, not that this helper populates the correct values, so a regression in those mappings (or in the camelCase doublePica conversion) would pass. Add explicit expected assertions for all groups and both properties for at least one representative size.

[emilysaffron]

lgtm, think the chromatic diff is acceptable - looks like just a classname change?

[pvaliani]

Thanks for addressing the feedback! Looks awesome