Skip to content

Make header CTA button purple - #26

Closed
davidrsch wants to merge 1 commit into
mainfrom
shinybot/in-the-header-component-change-the-prima-1785423126
Closed

Make header CTA button purple#26
davidrsch wants to merge 1 commit into
mainfrom
shinybot/in-the-header-component-change-the-prima-1785423126

Conversation

@davidrsch

Copy link
Copy Markdown

What changed

The Header component's primary button (.cta-button, the "Let's Talk" CTA in app/view/mod_header.R) is now purple instead of the app's primary blue.

  • config.yml — added a purple: "#7c3aed" entry to branding.colors. All colors in this app are config-driven and surfaced as CSS custom properties by generate_css_variables() in app/logic/general_utils.R, so this follows the existing convention rather than hardcoding a hex (or a named color, which Sass linting would flag) in the stylesheet.
  • app/styles/_header.scss.header .right .cta-button background changed from var(--primary) to var(--purple).
  • app/static/css/app.min.css — the committed build artifact is what the app actually serves, so the corresponding rule was updated to match.

Why this shape

The request was scoped to the Header component, so --primary itself was deliberately left untouched: it is still used by app/styles/_logs.scss and by the empty-state illustrations in app/main.R, none of which should change. .cta-button is used only by the Header module (verified — app/styles/_header.scss and app/view/mod_header.R are the only references), so the blast radius is exactly the header button.

How it was tested

  • Recompiled app/styles/main.scss with dart-sass (--style=compressed) and diffed the output against the pre-change compile: the single change is var(--primary)var(--purple) inside the .header .right .cta-button rule, with no other rules affected. app/static/css/app.min.css was hand-patched to that exact value rather than regenerated wholesale, because the locally available sass version emits cosmetically different output (e.g. white#fff, @import spacing) and a full rebuild would have buried a one-token change in unrelated churn. CI runs rhino::build_sass() itself, so it will regenerate the artifact from the SCSS in any case.
  • Confirmed config.yml still parses and that branding.colors resolves with purple: #7c3aed alongside an unchanged primary: #0099f9.
  • rhino::test_r() / rhino::lint_sass() / Cypress were not run: the renv packages recorded in renv.lock (including rhino and testthat) are not installed in this environment, and the Cypress suite needs a running app with Posit Connect access. No R or JS code changed, so the R unit tests are unaffected. No test was added — the repo has no styling assertions (tests/cypress/e2e/app.cy.js only checks that the app starts), and adding an e2e assertion that could not be run here risked landing a red test in this repo.

🤖 Opened by the shinybot-bridge mediforce workflow, on behalf of a shinybot chat request.

Add a `purple` branding color to config.yml and point the header's
`.cta-button` background at `var(--purple)` instead of `var(--primary)`,
so only the Header component changes. Rebuild the committed
app/static/css/app.min.css rule to match.
@davidrsch

Copy link
Copy Markdown
Author

🤖 Automated review by shinybot-bridge — flag false positives so we can tune the prompt.

Summary

3 files, +3/-2. .header .right .cta-button background moves from var(--primary) to a new config-driven var(--purple) (#7c3aed); config.yml gains the token; the committed app.min.css artifact is patched to match.

Verified independently against main:

  • Variable resolves at runtime. config.yml is the only source of :root vars — generate_css_variables() (app/logic/general_utils.R:52) maps over names(config$colors) and app/main.R:28 injects the result inline. app/styles/_colors.scss defines no static :root, so there is no second source of truth to keep in sync. --purple will resolve; no risk of an unset var collapsing the button background.
  • Blast radius is exactly the header. .cta-button is referenced only by app/styles/_header.scss and app/view/mod_header.R:40. --primary is untouched and still drives app/styles/_logs.scss:37 and the empty-state illustrations (app/main.R:94,102).
  • Build artifact is consistent, not drifted. git diff --word-diff on app/static/css/app.min.css shows a single token changed, in the matching selector — so the artifact's relationship to the SCSS is preserved exactly relative to main. CI's Build Sass step (.github/workflows/rhino-test.yml) regenerates it anyway.
  • No test breakage. tests/testthat/test-general_utils.R:78 asserts generate_css_variables() against a two-color fixture config, not the real config.yml, so the added key cannot break it. tests/cypress/e2e/app.cy.js only asserts the app starts.
  • No accessibility regression. White-on-#7c3aed is ≈5.7:1 contrast, up from ≈3.0:1 on #0099f9 — the change improves the CTA's AA standing.
  • No security surface. Diff touches no workflow, secret, auth, or input-handling code; the only added value is a hex literal.

Findings

Minor

  • config.yml:18 — the token is named for its hue (purple), not its role. The palette already mixes both conventions (red/grey1 vs primary/grey-text), so this isn't a violation — but a hue-named token means the next color tweak has to edit both config.yml and _header.scss, which is the coupling the config-driven design exists to avoid. cta or accent would let future changes be a one-line config edit.
  • app/static/css/app.min.css:1 — hand-patched rather than regenerated via rhino::build_sass(). Correct here (verified above), and the deviation is explained in the PR body, but worth noting CI does not assert the committed artifact matches the SCSS, so this class of edit is only ever verified by review.

Nit

  • No test added. Justified — the repo has no styling assertions and an unrunnable Cypress assertion would land red. Flagging only so the gap is on the record, not as something to fix in this PR.

Questions for the requester

  1. Request said "for a quick visual test" — this PR lands a permanent branding token plus a rebuilt artifact on the path to the default branch. Is the intent to merge, or to keep this as a throwaway preview branch?
  2. "the primary button color" is ambiguous: it could have meant the primary/main button (implemented reading — header CTA only) or the --primary color itself (which would recolor logs links and empty states too). The narrow reading is the safer one and is documented in the PR body — please confirm it matches intent.

Verdict

APPROVE (with minor comments)

No blockers, no majors. The change is minimal, resolves correctly at runtime, is contained to the requested component, breaks no test, and touches nothing security-sensitive. The two minors are judgement calls, and the questions are about intent rather than defects — none of them need a revision pass unless the answer to Q1 or Q2 changes the target.

@davidrsch

Copy link
Copy Markdown
Author

A hackatho test

@davidrsch davidrsch closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant