From 7e6e992a5112297aababf7842439e0ffa41f0ec1 Mon Sep 17 00:00:00 2001 From: Evan Bonsignori Date: Wed, 16 Sep 2026 21:24:53 +0000 Subject: [PATCH] Fix CTA button contrast and spacing (#63324) --- .../billing/cta-ghec-cost-centers.md | 2 +- data/variables/secret-scanning.yml | 5 +- .../stylesheets/markdown-overrides.scss | 78 +++++++++++++++++++ ...ge-with-permissions-and-product-callout.md | 12 +++ src/fixtures/tests/playwright-a11y.spec.ts | 7 ++ .../article/ArticleInlineLayout.tsx | 8 +- .../article/ViewMarkdownButton.module.scss | 33 +++++++- .../stylesheets/article-link-overrides.scss | 12 ++- 8 files changed, 151 insertions(+), 6 deletions(-) diff --git a/data/reusables/billing/cta-ghec-cost-centers.md b/data/reusables/billing/cta-ghec-cost-centers.md index f68d02c4b962..0c3cbd10b389 100644 --- a/data/reusables/billing/cta-ghec-cost-centers.md +++ b/data/reusables/billing/cta-ghec-cost-centers.md @@ -1,2 +1,2 @@ Cost centers are available with {% data variables.product.prodname_enterprise %} -Set up a trial of {% data variables.product.prodname_ghe_cloud %} {% octicon "link-external" height:16 aria-label="link-external" %} +
Set up a trial of {% data variables.product.prodname_ghe_cloud %} {% octicon "link-external" height:16 aria-label="link-external" %} diff --git a/data/variables/secret-scanning.yml b/data/variables/secret-scanning.yml index d9835c4eebe0..85538af0f059 100644 --- a/data/variables/secret-scanning.yml +++ b/data/variables/secret-scanning.yml @@ -20,7 +20,10 @@ pricing-calculator: 'pricing calculator' # Secret risk assessment call to action links. If changing the links below, also update the hard-coded link in /code-security/index.md secret-risk-assessment-cta-link: 'https://github.com/get_started?with=risk-assessment' secret-risk-assessment-cta-text: 'Run a security risk assessment' -secret-risk-assessment-cta-product: '[{% data variables.secret-scanning.secret-risk-assessment-cta-text %}](https://github.com/get_started?with=risk-assessment)' +# Keep this CTA as a raw , matching every other button in the content. The button +# styles are all qualified `a.btn`, so the `[LABEL](URL)` form puts +# the classes on an inner span and the button silently misses them. +secret-risk-assessment-cta-product: '{% data variables.secret-scanning.secret-risk-assessment-cta-text %}' # Combined to provide a secret to demonstrate push protection. Dummy secret, no access. learner-example-secret-a: 'secret_scanning_ab85fc6f8d76' diff --git a/src/content-render/stylesheets/markdown-overrides.scss b/src/content-render/stylesheets/markdown-overrides.scss index 6bd3014a52d7..0c4af7f5909f 100644 --- a/src/content-render/stylesheets/markdown-overrides.scss +++ b/src/content-render/stylesheets/markdown-overrides.scss @@ -55,3 +55,81 @@ margin-inline-start: 0.5rem; // Additional spacing to prevent bullet collision (direct children only) } } + +// A CTA button written on its own line in markdown — `` — becomes its own

, and that paragraph already carries the 16px +// rhythm margin. The `mt-3` utility then stacks a second 16px inside it, so the +// button ends up 32px below the preceding line but only 16px above the next one. +// Drop the utility when the button is alone in its paragraph and let the +// paragraph margin do the spacing, which puts the CTA on the same rhythm as +// every other block. `!important` is required because Primer's spacing +// utilities are themselves !important. +// +// `:only-child` is doing real work here — it is what keeps the two cases apart: +// - CTA callouts (`product:`/`permissions:` frontmatter) put the button after +// a
INSIDE the prose paragraph, so there is no paragraph margin above +// it and `mt-3` is the only thing separating it from the text. +// - The side-by-side Yes/No `.btn-outline` pairs are two buttons in one +// paragraph. +// Neither is an only child, so both keep their margin. +.markdown-body p > a.btn:only-child { + margin-top: 0 !important; +} + +// @primer/css holds `.btn` at `white-space: nowrap`, which a button cannot +// honour and still stay inside a narrow column. The longest CTA label — "Set up +// a trial of GitHub Enterprise Cloud", 322px — is wider than the article column +// below a ~420px viewport and wider than the callout's text column below ~390px, +// so the button ran past the content edge and was clipped. +// +// Letting the label wrap fixes it with no breakpoint to guess at. An +// inline-block is shrink-to-fit — min(max-content, available) — so +// `white-space: normal` changes nothing until max-content exceeds the space +// available: at every width where the button already fits it still renders on +// one line, byte-identical. That also makes it self-correcting for longer +// translated labels and for the narrower column a callout gives the same button. +.markdown-body a.btn, +.permissions-statement a.btn, +.product-statement a.btn { + white-space: normal; + + // Wrapping alone orphaned the trailing octicon on a line of its own: the + // space between the label and the icon is a valid break point, and the + // label filled the first line exactly. Laying the button out as a flex row + // instead lets the label wrap within itself and keeps the icon beside it, + // vertically centred. At widths where nothing wraps the result is within a + // pixel of the inline-block it replaces: same 17px left inset, same 21px + // right inset, same 32px height, still one line. The `gap` below covers the + // one thing that does change. + display: inline-flex; + align-items: center; + + // Flex layout eats the one thing that was separating the label from the icon. + // The markup is `Label {% octicon "link-external" %}`, and that + // literal space does survive Liquid and the markdown pipeline as a real text + // node — but a whitespace-only text node between two flex items is not itself + // a flex item, so no box is generated for it and the label ends up touching + // the icon. `gap` puts the space back. + // + // 4px rather than the measured width of that space glyph, because a space is + // font- and locale-dependent — it measures differently on two machines here — + // while 4px is the value Primer itself already uses between a button's icon + // and its label. The button ends up a fraction of a pixel wider than it was + // rather than most of a space narrower, on a number the design system owns. + // + // Only the label/icon gap is restored. Primer's `.btn .octicon` also carries + // `margin-right: 4px`, which assumes a LEADING icon and so lands outside the + // trailing icon on these CTAs, giving them 21px of inset on the right against + // 17px on the left. That asymmetry is what ships today, so it stays — zeroing + // it would restyle every CTA on the site, which is a different change from + // keeping a long label inside its column. + gap: 4px; + + // The octicon is a flex item now, and flex items shrink before their container + // overflows. Once the label wraps, the icon is the only thing left to give, so + // the 16px glyph was rendering at 11px in a 240px callout column. It is a + // fixed-size icon; the label is what should absorb a narrow column. + .octicon { + flex-shrink: 0; + } +} diff --git a/src/fixtures/fixtures/content/get-started/foo/page-with-permissions-and-product-callout.md b/src/fixtures/fixtures/content/get-started/foo/page-with-permissions-and-product-callout.md index 97bc2d9dcc72..ae96b51e9b8c 100644 --- a/src/fixtures/fixtures/content/get-started/foo/page-with-permissions-and-product-callout.md +++ b/src/fixtures/fixtures/content/get-started/foo/page-with-permissions-and-product-callout.md @@ -15,3 +15,15 @@ versions: Note that this page uses the `product` and `permissions` frontmatter property. So it should result in a call out box rendered with two messages. But only if the version is *not* Enterprise Server. + + + +Sign up for {% data variables.product.prodname_pages %} {% octicon "link-external" %} diff --git a/src/fixtures/tests/playwright-a11y.spec.ts b/src/fixtures/tests/playwright-a11y.spec.ts index e3e24039c38d..1c2017dce6ce 100644 --- a/src/fixtures/tests/playwright-a11y.spec.ts +++ b/src/fixtures/tests/playwright-a11y.spec.ts @@ -7,6 +7,13 @@ const SEARCH_TESTS = !!process.env.ELASTICSEARCH_URL const pages: { [key: string]: string } = { category: '/actions/category', codeAnnotations: '/get-started/markdown/code-annotations', + // The only fixture page that renders a CTA button. A `.btn-primary` anchor is the + // one shape the brand article-link override can drive under 4.5:1 — its label sits + // on a coloured fill rather than the page background — which is exactly what it did + // before `:not(.btn)` was added to + // src/frame/stylesheets/article-link-overrides.scss. Without this entry that + // exclusion has no test at all. + ctaButton: '/get-started/foo/page-with-permissions-and-product-callout', homepage: '/', learningPath: '/code-security/getting-started/quickstart?learn=foo_bar&learnProduct=code-security', diff --git a/src/frame/components/article/ArticleInlineLayout.tsx b/src/frame/components/article/ArticleInlineLayout.tsx index 0da5d1bf222c..c7db7db096bd 100644 --- a/src/frame/components/article/ArticleInlineLayout.tsx +++ b/src/frame/components/article/ArticleInlineLayout.tsx @@ -32,7 +32,13 @@ export const ArticleInlineLayout = ({ )} {introCallOuts && ( -

+ // `mt-4` (24px) matches the gap the grid layout gets from + // .belowIntroPlacement's own bottom margin. It is needed here because + // this layout puts the callouts in a separate wrapper from the intro, + // so the copy-markdown control is the last child of ITS wrapper and + // that rule cannot reach across. Without it the control sat flush on + // the callout box's top border. +
{introCallOuts}
)} diff --git a/src/frame/components/article/ViewMarkdownButton.module.scss b/src/frame/components/article/ViewMarkdownButton.module.scss index 8bd39057b5d6..220d7d8ac000 100644 --- a/src/frame/components/article/ViewMarkdownButton.module.scss +++ b/src/frame/components/article/ViewMarkdownButton.module.scss @@ -108,6 +108,37 @@ // The control's placement below the article lede. The margin sets the gap from // the lede — the Lead's own `mb-3` collapses into it, so this value wins — while // the header's bottom padding provides the space down to the rule beneath. +// +// Both gaps are spelled as Primer's step-4 spacer rather than a bare `1.5rem`. +// The bottom one has to be: the inline layout sets the same gap from TSX with +// `mt-4`, which compiles to exactly this `var(--base-size-24, 24px)`, and +// `1.5rem` and `mt-4` grep as nothing in common. The top one follows so that +// "match the gap above" below stays literally true. This is the one token here +// that is not `--brand-*`, and it still resolves: brand's main.css defines the +// whole `--base-size-*` scale at `:root` (@primer/primitives' size.css, its +// other home, is one of the files this app never imports). .belowIntroPlacement { - margin-top: 1.5rem; + margin-top: var(--base-size-24, 24px); + + // ...but the header's bottom padding only reaches the control when the + // control is the last thing in the header. On pages that also render intro + // callouts — the "Who can use this feature?" box, or the platform/tool + // pickers — those are siblings that follow it, and the control sat flush + // against the box's top border with no gap at all. Match the gap above, so + // the control is evenly spaced between the lede and whatever follows. + // + // `:not(:last-child)` keeps this off the pages where nothing follows, which + // already get their spacing from the header padding and would otherwise gain + // a second 24px. + // + // This rule only ever fires in the GRID layout, where ArticlePage hands the + // control and the callouts to `intro` as siblings. The inline layout puts + // them in two separate wrappers, so the control is the last child of its own + // and this selector cannot reach across — ArticleInlineLayout.tsx sets the + // identical gap by hand with `mt-4` there instead. The two are one visual gap + // and have to stay equal; nothing enforces that but this note and the shared + // token spelling, so change both or neither. + &:not(:last-child) { + margin-bottom: var(--base-size-24, 24px); + } } diff --git a/src/frame/stylesheets/article-link-overrides.scss b/src/frame/stylesheets/article-link-overrides.scss index ddad1dcf690f..47ea20966506 100644 --- a/src/frame/stylesheets/article-link-overrides.scss +++ b/src/frame/stylesheets/article-link-overrides.scss @@ -23,13 +23,21 @@ // semantic pair keeps this independent of any ancestor that re-maps the // component token — breadcrumbs-overrides.scss does exactly that. // - // Two exclusions, both because this selector outweighs the rules that + // Three exclusions, all because this selector outweighs the rules that // currently keep those anchors uncoloured: // - `[href]` — @primer/css holds `.markdown-body a:not([href])` at // `color: inherit`, for the bare named anchors markdown emits. // - `:not(.heading-link)` — heading anchors wrap the entire heading text, // and headings.scss holds them at `color: unset`. - a[href]:not(.heading-link) { + // - `:not(.btn)` — CTA buttons in content are plain anchors carrying + // @primer/css's button classes (``), so they + // match this rule too. `.btn-primary`'s own `color` is only (0,1,0) + // against this selector's (1,4,1), so without the exclusion the label + // painted brand link-blue on the green button — #005dd5 on #1f883d is + // 1.31:1, well under the 4.5:1 WCAG AA floor + // (github/technical-content#7679). Excluding `.btn` + // hands every button variant back to its own Primer colour tokens. + a[href]:not(.heading-link):not(.btn) { // Fallback literals are brand's LIGHT values, not Primer's. color: var(--brand-color-text-link-rest, #005dd5);