Skip to content

Make header "Let's Talk" button green - #28

Open
davidrsch wants to merge 1 commit into
mainfrom
shinybot/change-the-let-s-talk-button-in-the-head-1785494972
Open

Make header "Let's Talk" button green#28
davidrsch wants to merge 1 commit into
mainfrom
shinybot/change-the-let-s-talk-button-in-the-head-1785494972

Conversation

@davidrsch

Copy link
Copy Markdown

What

The .cta-button (the "Let's Talk" actionLink in the top-right of the header) now uses the themed green background instead of the primary blue.

  • app/styles/_header.scss: .header .right .cta-button background changed from var(--primary) to var(--green).
  • app/static/css/app.min.css: corresponding token updated in the committed compiled stylesheet.

Why

Requested via shinybot chat: the header CTA should appear green.

var(--green) (#3a5a40, defined under branding.colors in config.yml and emitted as a CSS custom property by generate_css_variables()) was used rather than a hardcoded color, so the button stays consistent with the config-driven theming added in #15 and remains overridable per deployment. The existing white label text keeps a strong contrast ratio (~8:1) on that green.

How it was tested

No automated test covers button styling (the R tests cover utils; the Cypress suite only asserts the app starts), and the app itself couldn't be launched here — rhino/renv packages are not installed in this environment.

Verification done instead: compiled app/styles/main.scss with sass --style=compressed before and after the change and diffed the output — the only difference is .header .right .cta-button{background:var(--primary)}{background:var(--green)}, which matches the one-token edit made to the committed app.min.css. (The compiled file was originally built with an older sass version, so it was patched surgically rather than fully regenerated, to avoid unrelated minifier churn; CI's rhino::build_sass() regenerates it either way.)


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

Use the themed --green color from config.yml branding for the
.cta-button background instead of --primary.
@davidrsch

Copy link
Copy Markdown
Author

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

Summary

Two-token diff, exactly matching the request. app/styles/_header.scss:33 background: var(--primary)var(--green), plus the same token in the committed app/static/css/app.min.css. No scope creep; nothing touching CI, auth, secrets, R logic, or dependencies.

Verified locally:

  • .cta-button has exactly one consumer — the Let's Talk actionLink in app/view/mod_header.R:43-44, inside div(class = "right header-section"). Selector .header .right .cta-button still matches; blast radius is the intended button only.
  • --green: "#3a5a40" really is defined under branding.colors in config.yml and emitted by generate_css_variables() (app/logic/general_utils.R:51), so the button stays config-overridable. Contrast of white on #3a5a407.7:1 — WCAG AAA. PR body's "~8:1" checks out.
  • Recompiled app/styles/main.scss with sass --style=compressed and diffed against the committed app.min.css: the hand-patched rule is byte-faithful modulo minifier-version normalisation (color:white vs color:#fff), and the patched file stays stylistically consistent with the rest of the committed artifact. The surgical patch was the right call over a full regen.

Findings

Minor

  • app/styles/_header.scss:33--green is a status token, not a brand/CTA token. It also drives .green-text / log-success styling (.green-text{color:var(--green)} in the compiled sheet). A deployment retuning "success green" in config.yml now silently restyles the header CTA too. Consider adding a dedicated cta (or secondary) entry to branding.colors and using var(--cta) here — same config-driven spirit as Feat: Add Theming with config.yml #15, without the semantic coupling. Not a blocker: --green is currently the only green token, so this is the correct choice given today's palette.
  • app/static/css/app.min.css:1pre-existing drift in the committed build artifact (verified: git show origin/main:app/static/css/app.min.css | grep -c logs-llm-placeholder1, while grep -rn logs-llm-placeholder app/styles/ → no matches; predates this branch). The committed CSS contains .logs-llm-placeholder img{width:50%} with no Sass source, so rhino::build_sass() cannot reproduce it — a full regen would delete that rule. This PR correctly avoided that by patching one token, but it means the artifact can't be safely rebuilt. Worth a separate issue to restore or drop the missing source. CI won't catch it: rhino-test.yml runs build_sass() but never checks the tree for a dirty diff.

Nit

  • No test added. Accurate per the PR body — tests/testthat/ covers logic utils only and tests/cypress/e2e/app.cy.js is a bare it('starts', () => {}). Adding a computed-style assertion for a one-token colour change would be over-engineering; fine as-is.

Verdict

APPROVE — no blockers, no majors. The two minors are follow-ups, not merge gates.

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