fix(features): darken placeholder text and move copy inside the value editor field - #8446
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change updates light-theme tertiary text tokens and react-select placeholder styling. It refactors Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to This change improves ValueEditor copy interaction, placeholder contrast, and component stories, but the new story may not compile and may expose inaccessible label relationships. Resolve these issues before merging. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
front-end
talissoncosta
changed the title
ca926e8 to
e86fc77
Compare
Docker builds report
|
❌ private-cloud · depot-ubuntu-latest-16 — run #20172 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/change-request-test.pw.ts › Change Request Tests › Change requests can be created, approved, and published with four-eyes approval @enterprise 🗂️ Previous results✅ oss · depot-ubuntu-latest-arm-16 — run #20172 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20172 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20174 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20174 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20174 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20174 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20173 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20173 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20173 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20173 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20171 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
|
Visual Regression15 screenshots compared. See report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: ff3bebd2-8a7a-4e31-8d11-e0050466b8a4
⛔ Files ignored due to path filters (1)
frontend/documentation/TokenReference.generated.stories.tsxis excluded by!**/*.generated.*
📒 Files selected for processing (9)
frontend/.storybook/preview.jsfrontend/common/theme/tokens.jsonfrontend/common/theme/tokens.tsfrontend/documentation/components/ValueEditor.stories.tsxfrontend/web/components/ValueEditor.jsfrontend/web/styles/3rdParty/_hljs.scssfrontend/web/styles/3rdParty/_react-select.scssfrontend/web/styles/_tokens.scssfrontend/web/styles/_variables.scss
💤 Files with no reviewable changes (1)
- frontend/web/styles/_variables.scss
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
e86fc77 to
a7d2438
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: 5211892c-06bb-485e-a892-cd9574dd6352
📒 Files selected for processing (1)
frontend/documentation/components/ValueEditor.stories.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
7c79b87 to
bb4e89c
Compare
bb4e89c to
201b683
Compare
Placeholders across inputs, textareas and the value editor all resolve to --color-text-tertiary. In light mode that was Neutrals/300 (#9da4ae), which is 2.26:1 on surface-default, well under the 4.5:1 AA threshold. Repoint the light value to Neutrals/500 (#656d7b), measured at 5.22:1. Dark mode already passed and is unchanged. Also switch the react-select placeholder onto the same token: it was the one placeholder still reading the raw SCSS variable, fixed for both themes. That leaves $input-placeholder-color(-dark) unused, so remove them. Closes #8441 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copy was the last item of the .txt/.json/.xml/.toml/.yaml row that floats above the editor, which conflated picking a format with copying the value. It now sits inside the input, right aligned, per the design. While moving it, make it a real control: BareButton with an aria-label instead of a <span onMouseDown>, so it is focusable and reachable by keyboard and screen reader. Its colour comes from --color-icon-action on hover rather than a hardcoded purple. Copy stayed hidden on disabled editors and under onlyOneLang only because it lived in the row those hide. That behaviour is preserved here so the change stays visual, but it is worth revisiting: three of the eight call sites are permanently disabled read-only values, which is where copy is most useful. Drop the ConfigProvider wrapper. ValueEditor reads none of isLoading, error, getValue or hasFeature, and on the E2E path they were spread onto the textarea as unknown DOM attributes. It also crashed the component outside app boot, which is what the new story surfaced. Swap the validation icons from ionicons to our own Icon. Storybook stubs IonIcon as a grey circle, so the new story was showing a placeholder where a warning triangle belongs. Both ids stay: saveFeatureWithValidation reads the error one off the DOM. Add a Storybook story covering the empty, filled, multiline, JSON, invalid JSON, code-medium, disabled and single-language states, snapshotted by Chromatic in both themes. Closes #8442 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The design puts the info icon at 20x20 beside the label; ours asked for 12,
and since info-outlined draws its circle across 20 of a 24 viewBox that
rendered a 10px target. It is 16 now.
It also sat 2px above the text. .control-label is display: block, so the
tooltip's inline-flex wrapper took part in inline layout, and an
inline-flex box whose only child is a replaced element baselines on its
bottom margin edge rather than its content. The svg { vertical-align:
bottom } rule alongside it cannot correct that: vertical-align is ignored
on flex items.
Laying the label out as a centred flex row puts the icon dead on the text
centre. vertical-align: middle was the other candidate and overshoots by
1.3px, aligning to half the x-height rather than the cap centre.
That goes on FieldLabel's own element, not on .control-label. Nine
hand-written labels across six other files carry that class, none of them
with an icon to align, so they keep display: block and this touches only
what FieldLabel renders. The stylesheet is unchanged.
Checked against block: the label keeps its full width, and a long label
still wraps to the same height.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
201b683 to
f9a428e
Compare
|
@themis-blindfold review |
talissoncosta
changed the title
⚖️ Themis review: ✅ Ship itThe placeholder token now meets the requested light-theme contrast target, the copy control is a keyboard-accessible button inside the value editor, and the shared label icon aligns with the design. Completed frontend, unit, end-to-end, Chromatic, lint, and security checks are green.
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: Solid accessibility and usability improvement in a high-frequency editor, with a small, contained visual footprint. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The copy button has finally found its home · reviewed at f9a428e |
talissoncosta
changed the title
3 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #8441
Closes #8442
Both from the same design.
--color-text-tertiary, which was Neutrals/300 in light mode: 2.26:1 on white. Now Neutrals/500, measured at 5.22:1. Dark mode already passed. The same token also fixes two hint texts that were failing for the same reason.react-select's placeholder was the last one reading raw SCSS instead of the token, so it moves across too..txt/.json/.xml/.toml/.yamlrow. It is a real<button>with anaria-labelnow, so it is keyboard reachable.Screenshots
How did you test this code?
New Storybook story (
Components/Forms/ValueEditor) covering the empty, filled, multiline, JSON, invalid JSON, code-medium and disabled states, snapshotted by Chromatic in both themes.Contrast measured in the browser rather than calculated: computed
coloragainst computedbackground-coloron the rendered placeholder.Manually through Create feature → Value, MV variation values, segment overrides and the SAML metadata field.
Worth a look
FieldLabelis shared, so the icon change touches every form label in the app. The flex sits onFieldLabel's own element rather than.control-label, since nine hand-written labels elsewhere use that class and have no icon to align.enter a value; we renderEnter a value...fromHighlight's hardcoded default. Left alone as a copy change for @dragos-bubu to confirm.