Skip to content

Commit 864277c

Browse files
joshwoodingjoshwooding
committed
Fix read-only Input zero value and align Dropdown validation precedence
- Input: treat numeric 0 as a real value when read-only instead of showing the empty read-only marker - PillInput: same read-only empty-value handling for consistency (internal) - Dropdown: FormField validation status now takes precedence over the local validationStatus prop, matching the other form controls - Add e2e tests for Input zero value, Dropdown validation precedence, and ComboBox read-only marker
1 parent 7b4c73e commit 864277c

13 files changed

Lines changed: 222 additions & 15 deletions

File tree

‎.changeset/dropdown-validation-precedence.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@salt-ds/core": patch
3+
---
4+
5+
Fixed `Dropdown` validation status precedence to align with the other form controls, so the `FormField` validation status now takes precedence over the `Dropdown`'s own `validationStatus` prop.

‎.changeset/input-readonly-zero-value.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@salt-ds/core": patch
3+
"@salt-ds/lab": patch
4+
---
5+
6+
Fixed read-only inputs so empty controlled values, empty default values, and empty selections consistently display the `emptyReadOnlyMarker` (`—` by default), while non-empty values such as numeric `0` continue to display the real value.

‎packages/core/src/__tests__/__e2e__/combo-box/ComboBox.cy.tsx‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { CustomFloatingComponentProvider, FLOATING_TEST_ID } from "../common";
77
const {
88
Default,
99
Readonly,
10+
ReadonlyEmpty,
1011
WithDefaultSelected,
1112
Disabled,
1213
DisabledOption,
@@ -903,4 +904,16 @@ describe("Given a ComboBox", () => {
903904
cy.findByRole("combobox").should("not.have.attr", "aria-describedby");
904905
cy.findByRole("combobox").should("not.have.attr", "aria-labelledby");
905906
});
907+
908+
describe("WHEN read only", () => {
909+
it("THEN should show the selected value rather than the empty marker", () => {
910+
cy.mount(<Readonly />);
911+
cy.findByRole("textbox").should("have.value", "California");
912+
});
913+
914+
it("THEN should show the empty marker when there is no selection", () => {
915+
cy.mount(<ReadonlyEmpty />);
916+
cy.findByRole("textbox").should("have.value", "—");
917+
});
918+
});
906919
});

‎packages/core/src/__tests__/__e2e__/dropdown/Dropdown.cy.tsx‎

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Dropdown, Option } from "@salt-ds/core";
1+
import { Dropdown, FormField, FormFieldLabel, Option } from "@salt-ds/core";
22
import * as dropdownStories from "@stories/dropdown/dropdown.stories";
33
import { composeStories } from "@storybook/react-vite";
44
import { type KeyboardEventHandler, useRef, useState } from "react";
@@ -225,6 +225,36 @@ describe("Given a Dropdown", () => {
225225
cy.findByRole("combobox").should("have.text", "California");
226226
});
227227

228+
it("should show the empty marker when it is readonly with no selection", () => {
229+
cy.mount(
230+
<Dropdown readOnly>
231+
<Option value="Alabama" />
232+
</Dropdown>,
233+
);
234+
235+
cy.findByRole("combobox").should("have.text", "—");
236+
});
237+
238+
it("should show the empty marker when it is readonly with a controlled empty value", () => {
239+
cy.mount(
240+
<Dropdown readOnly value="">
241+
<Option value="Alabama" />
242+
</Dropdown>,
243+
);
244+
245+
cy.findByRole("combobox").should("have.text", "—");
246+
});
247+
248+
it("should show custom value text rather than the empty marker when it is readonly", () => {
249+
cy.mount(
250+
<Dropdown readOnly value="Custom value">
251+
<Option value="Alabama" />
252+
</Dropdown>,
253+
);
254+
255+
cy.findByRole("combobox").should("have.text", "Custom value");
256+
});
257+
228258
it("should not receive focus via tab if it is disabled", () => {
229259
cy.mount(
230260
<div>
@@ -526,4 +556,31 @@ describe("Given a Dropdown", () => {
526556
.should("exist")
527557
.and("have.attr", "role", "listbox");
528558
});
559+
560+
describe("validation status", () => {
561+
it("should use its own validation status when not in a FormField", () => {
562+
cy.mount(
563+
<Dropdown validationStatus="warning">
564+
<Option value={1}>1</Option>
565+
</Dropdown>,
566+
);
567+
cy.findByRole("combobox").should("have.class", "saltDropdown-warning");
568+
});
569+
570+
it("should prioritize the FormField validation status over its own", () => {
571+
cy.mount(
572+
<FormField validationStatus="error">
573+
<FormFieldLabel>Field</FormFieldLabel>
574+
<Dropdown validationStatus="warning">
575+
<Option value={1}>1</Option>
576+
</Dropdown>
577+
</FormField>,
578+
);
579+
cy.findByRole("combobox").should("have.class", "saltDropdown-error");
580+
cy.findByRole("combobox").should(
581+
"not.have.class",
582+
"saltDropdown-warning",
583+
);
584+
});
585+
});
529586
});

‎packages/core/src/__tests__/__e2e__/input/Input.cy.tsx‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,11 +170,33 @@ describe("GIVEN an Input", () => {
170170
cy.findByRole("textbox").should("have.value", "—");
171171
});
172172

173+
it("THEN should show an emdash for an empty default value", () => {
174+
cy.mount(<Input defaultValue="" readOnly />);
175+
cy.findByRole("textbox").should("have.value", "—");
176+
});
177+
178+
it("THEN should show an emdash for a controlled empty value", () => {
179+
cy.mount(<Input value="" readOnly />);
180+
cy.findByRole("textbox").should("have.value", "—");
181+
});
182+
173183
it("THEN should cy.mount an custom marker", () => {
174184
cy.mount(<Input emptyReadOnlyMarker="#" readOnly />);
175185
cy.findByRole("textbox").should("have.value", "#");
176186
});
177187
});
188+
189+
describe("AND the value is zero", () => {
190+
it("THEN should show the zero value rather than the empty marker", () => {
191+
cy.mount(<Input defaultValue={0} readOnly />);
192+
cy.findByRole("textbox").should("have.value", "0");
193+
});
194+
195+
it("THEN should show a controlled zero value rather than the empty marker", () => {
196+
cy.mount(<Input value={0} readOnly />);
197+
cy.findByRole("textbox").should("have.value", "0");
198+
});
199+
});
178200
});
179201

180202
describe("WHEN used in Formfield", () => {

‎packages/core/src/__tests__/__e2e__/number-input/NumberInput.cy.tsx‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1047,6 +1047,16 @@ describe("Number Input", () => {
10471047
cy.findByRole("textbox").should("have.value", "—");
10481048
});
10491049

1050+
it("should show emdash for an empty default value", () => {
1051+
cy.mount(<ReadOnly defaultValue="" readOnly />);
1052+
cy.findByRole("textbox").should("have.value", "—");
1053+
});
1054+
1055+
it("should show emdash for a controlled empty value", () => {
1056+
cy.mount(<ReadOnly value="" readOnly />);
1057+
cy.findByRole("textbox").should("have.value", "—");
1058+
});
1059+
10501060
it("should show a custom marker if provided when empty", () => {
10511061
cy.mount(
10521062
<ReadOnly defaultValue={undefined} readOnly emptyReadOnlyMarker="#" />,

‎packages/core/src/dropdown/Dropdown.tsx‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ export const Dropdown = forwardRef(function Dropdown<Item>(
177177

178178
const disabled = Boolean(disabledProp) || formFieldDisabled;
179179
const readOnly = Boolean(readOnlyProp) || formFieldReadOnly;
180-
const validationStatus = validationStatusProp ?? formFieldValidationStatus;
180+
const validationStatus = formFieldValidationStatus ?? validationStatusProp;
181181
const required = formFieldRequired
182182
? ["required", "asterisk"].includes(formFieldRequired)
183183
: requiredProp;
@@ -219,10 +219,9 @@ export const Dropdown = forwardRef(function Dropdown<Item>(
219219
const selectedValue = selectedState
220220
.map((item) => valueToString(item))
221221
.join(", ");
222-
const isEmptyReadOnly = readOnly && selectedValue === "";
223-
const valueText = isEmptyReadOnly
224-
? emptyReadOnlyMarker
225-
: (value ?? selectedValue);
222+
const displayedValue = value ?? selectedValue;
223+
const valueText =
224+
readOnly && displayedValue === "" ? emptyReadOnlyMarker : displayedValue;
226225

227226
const handleOpenChange: UseFloatingUIProps["onOpenChange"] = (
228227
newOpen,

‎packages/core/src/input/Input.tsx‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,16 @@ export interface InputProps
7474
bordered?: boolean;
7575
}
7676

77+
function isEmptyReadOnlyValue(
78+
value: InputProps["value"] | InputProps["defaultValue"],
79+
) {
80+
return (
81+
value == null ||
82+
value === "" ||
83+
(Array.isArray(value) && value.length === 0)
84+
);
85+
}
86+
7787
export const Input = forwardRef<HTMLDivElement, InputProps>(
7888
function Input(props, ref) {
7989
const { className, props: finalProps } = useClassNameInjection(
@@ -134,8 +144,16 @@ export const Input = forwardRef<HTMLDivElement, InputProps>(
134144

135145
const [focused, setFocused] = useState(false);
136146

137-
const isEmptyReadOnly = isReadOnly && !defaultValueProp && !valueProp;
138-
const defaultValue = isEmptyReadOnly
147+
const isValueEmptyReadOnly = isReadOnly && isEmptyReadOnlyValue(valueProp);
148+
const isDefaultValueEmptyReadOnly =
149+
isReadOnly && isEmptyReadOnlyValue(defaultValueProp);
150+
const controlledValue =
151+
valueProp === undefined
152+
? undefined
153+
: isValueEmptyReadOnly
154+
? emptyReadOnlyMarker
155+
: valueProp;
156+
const defaultValue = isDefaultValueEmptyReadOnly
139157
? emptyReadOnlyMarker
140158
: defaultValueProp;
141159

@@ -154,7 +172,7 @@ export const Input = forwardRef<HTMLDivElement, InputProps>(
154172
: inputPropsRequired;
155173

156174
const [value, setValue] = useControlled({
157-
controlled: valueProp,
175+
controlled: controlledValue,
158176
default: defaultValue,
159177
name: "Input",
160178
state: "value",

‎packages/core/src/pill-input/PillInput.tsx‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,16 @@ export interface PillInputProps
8585
bordered?: boolean;
8686
}
8787

88+
function isEmptyReadOnlyValue(
89+
value: PillInputProps["value"] | PillInputProps["defaultValue"],
90+
) {
91+
return (
92+
value == null ||
93+
value === "" ||
94+
(Array.isArray(value) && value.length === 0)
95+
);
96+
}
97+
8898
export const PillInput = forwardRef(function PillInput(
8999
{
90100
"aria-activedescendant": ariaActiveDescendant,
@@ -148,8 +158,18 @@ export const PillInput = forwardRef(function PillInput(
148158
const [focused, setFocused] = useState(false);
149159
const [focusedPillIndex, setFocusedPillIndex] = useState(-1);
150160

151-
const isEmptyReadOnly = isReadOnly && !defaultValueProp && !valueProp;
152-
const defaultValue = isEmptyReadOnly ? emptyReadOnlyMarker : defaultValueProp;
161+
const isValueEmptyReadOnly = isReadOnly && isEmptyReadOnlyValue(valueProp);
162+
const isDefaultValueEmptyReadOnly =
163+
isReadOnly && isEmptyReadOnlyValue(defaultValueProp);
164+
const controlledValue =
165+
valueProp === undefined
166+
? undefined
167+
: isValueEmptyReadOnly
168+
? emptyReadOnlyMarker
169+
: valueProp;
170+
const defaultValue = isDefaultValueEmptyReadOnly
171+
? emptyReadOnlyMarker
172+
: defaultValueProp;
153173

154174
const {
155175
"aria-describedby": inputDescribedBy,
@@ -167,7 +187,7 @@ export const PillInput = forwardRef(function PillInput(
167187
: inputPropsRequired;
168188

169189
const [value, setValue] = useControlled({
170-
controlled: valueProp,
190+
controlled: controlledValue,
171191
default: defaultValue,
172192
name: "Input",
173193
state: "value",

‎packages/date-components/src/__tests__/__e2e__/date-input/DateInputRange.cy.tsx‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,29 @@ describe("GIVEN a DateInputRange", () => {
169169
.date,
170170
};
171171

172+
it("SHOULD show the empty marker when read-only with an empty default value", () => {
173+
cy.mount(
174+
<DateInputRange
175+
defaultValue={{ startDate: "", endDate: "" }}
176+
readOnly
177+
/>,
178+
);
179+
180+
cy.findAllByRole("textbox").each(($input) => {
181+
cy.wrap($input).should("have.value", "—");
182+
});
183+
});
184+
185+
it("SHOULD show the empty marker when read-only with a controlled empty value", () => {
186+
cy.mount(
187+
<DateInputRange value={{ startDate: "", endDate: "" }} readOnly />,
188+
);
189+
190+
cy.findAllByRole("textbox").each(($input) => {
191+
cy.wrap($input).should("have.value", "—");
192+
});
193+
});
194+
172195
it("SHOULD have an accessible name via aria-labelledby when wrapped in a FormField", () => {
173196
cy.mount(
174197
<FormField>

0 commit comments

Comments
 (0)