Repository navigation
Pret UI: put the other field controls on Input's field style - #6871
burieberry wants to merge 2 commits into
Conversation
Preview deploymentsHost Test Results 1 files 1 suites 2h 14m 46s ⏱️ Results for commit 9d5777c. Realm Server Test Results 1 files ±0 363 suites ±0 1h 50m 22s ⏱️ - 20m 4s Results for commit 9d5777c. ± Comparison against earlier commit 37fe88b. |
f518d72 to
1ed7e8e
Compare
0422de6 to
69a22c1
Compare
6c75828 to
37fe88b
Compare
37fe88b to
9d5777c
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d5777c595
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .pretui-fieldw[data-invalid]:not(:has(:disabled, [aria-disabled='true'])) { | ||
| --input: var(--destructive-ink); |
There was a problem hiding this comment.
Limit disabled detection to the field control
When an invalid Field contains an enabled picker with a disabled option, this descendant-wide :has([aria-disabled='true']) becomes true and stops applying the error-edge token. For example, Autocomplete and TreeSelect render disabled options beneath the field while their popups are open (autocomplete.gts lines 771–819 and tree-select.gts lines 371–395), so merely opening such a picker makes the invalid border disappear even though the control remains enabled. Restrict this check to the actual control/trigger rather than every descendant.
Useful? React with 👍 / 👎.
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review went after the shared field style's state rules (invalid, disabled, focus) across Field and the select-type controls.
One blocking issue: an invalid Field loses its error edge whenever an enabled picker inside it opens a popup with a disabled option.
field.gts: the disabled exemption on the invalid rule matches descendants of the popup, not only the control (inline comment; this agrees with the existing Codex thread on the same line).
| hue misses a field boundary's 3:1) and no tint, through --input, | ||
| which the select-type triggers draw their edge from. A disabled | ||
| control can't be fixed, so it keeps the usual edge. */ | ||
| .pretui-fieldw[data-invalid]:not(:has(:disabled, [aria-disabled='true'])) { |
There was a problem hiding this comment.
[Claude Code 🤖] Blocking, regression. :has(:disabled, [aria-disabled='true']) matches any descendant of the Field, and TreeSelect's tree and Autocomplete's listbox render inline inside it with aria-disabled='true' on disabled rows. So an invalid Field holding an enabled TreeSelect or Autocomplete with one disabled option drops --input: var(--destructive-ink) the moment the popup opens, and the error edge comes back when it closes. Can the exemption ignore popup contents, e.g. :not(:has(:is(:disabled, [aria-disabled='true']):not([role='listbox'] *, [role='tree'] *))), or key off a marker the control's own trigger/input sets? A Field test with an open TreeSelect holding a disabled node would pin it.
Background and Goal
CS-14140. Input, Select and Autocomplete share one field style. This PR puts the rest of Pret UI's field controls on it, so they all match Input or Select side by side. The field style is:
--boxel-form-control-height-sm)calc(var(--radius) - 0.125rem))--boxel-font-size-xstext, at least 16px on touch screens--inputborder inside the box--ringfocus outline--boxel-disabled-opacityThis covers the typed inputs (EmailInput, UrlInput, SearchInput, PhoneInput, PasswordInput, NumberInput), Textarea, InputGroup, Combobox, Lookup, NativeSelect, MultiSelect and TreeSelect.
Every select-type control now shows Select's caret, flipped while it's open. An invalid control has a
--destructive-inkedge with no tint, whether the error comes from the control itself, a Field or a FormField. Field's and FormField's error messages drop the ✕ glyph, since the control's edge already shows the error, and FormField's message text is the severity's-ink, as on Input's error row. Each component's Theming doc is updated to match.Where to start
internal/field-dress.gts:FieldDressis the wrapper Input, Textarea and the typed inputs share. It sets BoxelInput's look entirely through--boxel-*variables, never reaching into boxel-ui's markup. It also holds the hover, focus, disabled and invalid rules.internal/caret.gts: the one chevron the select-type controls draw. Combobox and Lookup draw the same path on power-select's status icon.native-select.gts,multi-select.gtsandtree-select.gts: each trigger takes Select's style.field.gts,form-field.gtsandautocomplete.gts: the error states, matched to Input's. Field sets--inputon itself, so every select-type trigger inside an invalid Field takes the error edge.cue.gtsandfield-error.gts: a@glyphargument. Field turns Cue's glyph off; FormField turns off FieldError's error glyph only, so warnings and info keep theirs and severities stay apart without relying on color.field.examples.gts: the six select-type controls in a Field, with an Invalid switch.Key decisions and non-obvious mechanics
variables.cssgains--boxel-disabled-opacity: 0.45, so every Pret UI control dims by the same amount; CS-14339 updates ADR 009 to say the shared stylesheets take changes like this. Select's and Picker's triggers give theirCaretDownawidthandheight, so the caret doesn't render at full size before the stylesheet loads.outline-offset: -1px), which still shows in forced-colors mode.--input.@type='search'shareinternal/search-glyph.gts. It's a 14px magnifier that starts where text starts in the other fields, so neither needs BoxelInput's fixed 20px icon.--ringinside themselves on focus. Host's global button focus rule reads its offset from--host-outline-offset, so the buttons set that too.right: 5px. Combobox and Lookup reset that withright: autobefore settinginset-inline-end, so the caret sits at Select's inset in both text directions.🤖 Generated with Claude Code