Skip to content

Make the "Let's Talk" button green - #27

Closed
davidrsch wants to merge 1 commit into
mainfrom
shinybot/change-the-let-s-talk-button-color-to-gr-1785424951
Closed

Make the "Let's Talk" button green#27
davidrsch wants to merge 1 commit into
mainfrom
shinybot/change-the-let-s-talk-button-color-to-gr-1785424951

Conversation

@davidrsch

Copy link
Copy Markdown

What changed

The header CTA button ("Let's Talk") now uses the branding --green color for its background instead of --primary.

  • app/styles/_header.scss: .header .right .cta-button { background: var(--green); } (was var(--primary)).
  • app/static/css/app.min.css: the committed compiled stylesheet is what the app actually serves, so the corresponding rule was updated to match the SCSS source.

Why

Requested via shinybot chat: "Change the "Let's Talk" button color to green."

The button previously inherited --primary (#0099f9, Appsilon blue). It now uses the existing --green branding token (#3a5a40 by default in config.yml), so the color stays theme-driven and configurable rather than hardcoded, and white label text keeps good contrast. --primary itself was left untouched because it is also used elsewhere (e.g. app/styles/_logs.scss).

How it was tested

No automated tests were run: R package dependencies are not installed in the agent environment (renv reports missing packages, and neither rhino nor sass is available), so rhino::build_sass() and the testthat suite could not be executed. The compiled CSS was therefore edited by hand to exactly mirror the one-token SCSS change; the resulting rule is cta-button{background:var(--green);color:white;padding:10px;border-radius:10px;margin:0 10px}. No existing testthat or Cypress test asserts on the header CTA styling, and a two-line color change did not seem to warrant introducing a new visual-assertion test. Worth a quick visual confirmation on review.


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

Use the branding --green color for the header CTA background instead of
--primary, and update the compiled app.min.css to match.
@davidrsch

Copy link
Copy Markdown
Author

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

Summary

Diff is exactly what the request asked for and nothing more: 2 lines, app/styles/_header.scss + the committed app/static/css/app.min.css. No CI, secrets, auth, dependency or R-logic files touched, and no content from the chat request leaked into the repo. Verified against origin/main.

Checks I actually ran:

  • git diff main...HEAD --stat → 2 files, 1 insertion / 1 deletion each.
  • Word-diff of the minified CSS → the only token that changed is .cta-button{background:var(--primary)var(--green). The hand-edit faithfully mirrors the SCSS change, so the "couldn't run rhino::build_sass()" caveat in the PR body checks out.
  • --green is a real, defined token (config.yml branding.colors.green: "#3a5a40", emitted at :root by app/logic/general_utils.R:49-62), so the button won't render transparent.
  • Contrast of white on #3a5a40 ≈ 7.7:1 → passes WCAG AA/AAA for normal text.
  • .cta-button is used in exactly one place (app/view/mod_header.R:44); no other rule or inline style overrides its background.

Findings

Minor

  • app/styles/_header.scss:33--green is a status color, not a CTA/branding color. It's the log-severity palette: app/styles/_colors.scss:5-18 (.green-text, .green-highlight) and app/logic/logs_utils.R:49 map it to the INFO log state. The PR body calls it "the existing --green branding token", which overstates its intent. Practical consequence: a deployment that re-themes branding.colors.green to tune INFO-log styling now silently restyles the header CTA, and vice versa. A dedicated branding.colors.cta (defaulting to the same green) would decouple them — but that's a config addition for a one-line color request, so reusing the token is a defensible KISS call. Reviewer's judgement, not an auto-reject.
  • app/static/css/app.min.css:1 — generated file hand-edited without running the generator. rhino.yml sets sass: node, so this file is rhino::build_sass() output. CI (.github/workflows/rhino-test.yml:57) runs build_sass() but never asserts the working tree is clean afterwards, so SCSS↔CSS drift would pass CI silently and Cypress would test a freshly-built stylesheet rather than the committed one. In this PR the edit is provably equivalent (word-diff above), so it is not a blocker — but please run rhino::build_sass() once locally before merge to confirm the committed CSS isn't otherwise stale relative to current SCSS sources.

Nit

  • #3a5a40 is a dark, desaturated forest green. Contrast is fine, but "make it green" from a chat message may have meant something more obviously green. 10 seconds of visual confirmation settles it.
  • No test coverage — agreed with the implementer that a two-line color change doesn't warrant a new visual assertion. Worth noting the existing Cypress suite is it('starts', () => {}) (tests/cypress/e2e/app.cy.js:6), i.e. there is no UI assertion baseline to extend here anyway. Pre-existing, unrelated to this diff.

Verdict

APPROVE after a visual check of the new green. Scope is tight, the change is correct, the compiled-CSS shortcut verifies, and nothing security-relevant is touched. The two minor items are follow-ups, not revision blockers.

@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