-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: StudioCodeListEditor - display Numberfield or Checkbox based on value type #14398
base: main
Are you sure you want to change the base?
Changes from all commits
e9b29b2
d6c13ac
e32b242
e861d29
50ea9e5
9e9a73f
29079f6
fd25884
1724951
aa6c8d2
3aed7a8
0882627
8c3861d
80fdc7f
e5194f8
0bd270e
954bbbc
cf17f0b
0e73d8e
1d01ce3
3f0cdc3
4f1c8ba
6c4ad49
267d8de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,7 +3,7 @@ import type { CodeListItemValue } from '../types/CodeListItemValue'; | |||||||||||||||||
import { StudioInputTable } from '../../StudioInputTable'; | ||||||||||||||||||
import { TrashIcon } from '../../../../../studio-icons'; | ||||||||||||||||||
import type { FocusEvent, HTMLInputAutoCompleteAttribute, ReactElement } from 'react'; | ||||||||||||||||||
import React, { useCallback, useEffect, useRef } from 'react'; | ||||||||||||||||||
import React, { forwardRef, useCallback, useEffect, useRef } from 'react'; | ||||||||||||||||||
import { changeDescription, changeHelpText, changeLabel, changeValue } from './utils'; | ||||||||||||||||||
import { useStudioCodeListEditorContext } from '../StudioCodeListEditorContext'; | ||||||||||||||||||
import type { ValueError } from '../types/ValueError'; | ||||||||||||||||||
|
@@ -49,7 +49,7 @@ export function StudioCodeListEditorRow({ | |||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
const handleValueChange = useCallback( | ||||||||||||||||||
(value: string) => { | ||||||||||||||||||
(value: CodeListItemValue) => { | ||||||||||||||||||
const updatedItem = changeValue(item, value); | ||||||||||||||||||
onChange(updatedItem); | ||||||||||||||||||
}, | ||||||||||||||||||
|
@@ -66,7 +66,7 @@ export function StudioCodeListEditorRow({ | |||||||||||||||||
|
||||||||||||||||||
return ( | ||||||||||||||||||
<StudioInputTable.Row> | ||||||||||||||||||
<TextfieldCell | ||||||||||||||||||
<TypedInputCell | ||||||||||||||||||
autoComplete='off' | ||||||||||||||||||
error={error && texts.valueErrors[error]} | ||||||||||||||||||
label={texts.itemValue(number)} | ||||||||||||||||||
|
@@ -105,45 +105,139 @@ export function StudioCodeListEditorRow({ | |||||||||||||||||
); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
type TextfieldCellProps = { | ||||||||||||||||||
error?: string; | ||||||||||||||||||
label: string; | ||||||||||||||||||
onChange: (newString: string) => void; | ||||||||||||||||||
type TypedInputCellProps = { | ||||||||||||||||||
value: CodeListItemValue; | ||||||||||||||||||
label: string; | ||||||||||||||||||
onChange: (newValue: CodeListItemValue) => void; | ||||||||||||||||||
onFocus?: (event: FocusEvent) => void; | ||||||||||||||||||
autoComplete?: HTMLInputAutoCompleteAttribute; | ||||||||||||||||||
error?: string; | ||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
function TextfieldCell({ error, label, value, onChange, autoComplete }: TextfieldCellProps) { | ||||||||||||||||||
function TypedInputCell({ error, label, value, onChange, autoComplete }: TypedInputCellProps) { | ||||||||||||||||||
const ref = useRef<HTMLInputElement>(null); | ||||||||||||||||||
|
||||||||||||||||||
useEffect((): void => { | ||||||||||||||||||
ref.current?.setCustomValidity(error || ''); | ||||||||||||||||||
}, [error]); | ||||||||||||||||||
|
||||||||||||||||||
const handleChange = useCallback( | ||||||||||||||||||
(event: React.ChangeEvent<HTMLInputElement>): void => { | ||||||||||||||||||
onChange(event.target.value); | ||||||||||||||||||
}, | ||||||||||||||||||
[onChange], | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
const handleFocus = useCallback((event: FocusEvent<HTMLInputElement>): void => { | ||||||||||||||||||
event.target.reportValidity(); | ||||||||||||||||||
}, []); | ||||||||||||||||||
|
||||||||||||||||||
return ( | ||||||||||||||||||
<StudioInputTable.Cell.Textfield | ||||||||||||||||||
aria-label={label} | ||||||||||||||||||
autoComplete={autoComplete} | ||||||||||||||||||
className={classes.textfieldCell} | ||||||||||||||||||
onChange={handleChange} | ||||||||||||||||||
onFocus={handleFocus} | ||||||||||||||||||
ref={ref} | ||||||||||||||||||
value={(value as string) ?? ''} | ||||||||||||||||||
/> | ||||||||||||||||||
); | ||||||||||||||||||
switch (typeof value) { | ||||||||||||||||||
case 'number': | ||||||||||||||||||
return ( | ||||||||||||||||||
<NumberfieldCell | ||||||||||||||||||
label={label} | ||||||||||||||||||
value={value} | ||||||||||||||||||
autoComplete={autoComplete} | ||||||||||||||||||
onChange={onChange} | ||||||||||||||||||
onFocus={handleFocus} | ||||||||||||||||||
ref={ref} | ||||||||||||||||||
/> | ||||||||||||||||||
); | ||||||||||||||||||
case 'boolean': | ||||||||||||||||||
return ( | ||||||||||||||||||
<CheckboxCell | ||||||||||||||||||
label={label} | ||||||||||||||||||
value={value} | ||||||||||||||||||
onChange={onChange} | ||||||||||||||||||
onFocus={handleFocus} | ||||||||||||||||||
ref={ref} | ||||||||||||||||||
/> | ||||||||||||||||||
); | ||||||||||||||||||
default: | ||||||||||||||||||
return ( | ||||||||||||||||||
<TextfieldCell | ||||||||||||||||||
label={label} | ||||||||||||||||||
value={value} | ||||||||||||||||||
autoComplete={autoComplete} | ||||||||||||||||||
onChange={onChange} | ||||||||||||||||||
onFocus={handleFocus} | ||||||||||||||||||
ref={ref} | ||||||||||||||||||
/> | ||||||||||||||||||
); | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
const NumberfieldCell = forwardRef<HTMLInputElement, TypedInputCellProps>( | ||||||||||||||||||
({ label, value, onChange, onFocus, autoComplete }, ref) => { | ||||||||||||||||||
const handleNumberChange = useCallback( | ||||||||||||||||||
(numberValue: number): void => { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
if (numberValue === undefined) return; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When will this case happen, and why is the check not present in the others? Should we consider using the same pattern as in the |
||||||||||||||||||
onChange(numberValue); | ||||||||||||||||||
}, | ||||||||||||||||||
Comment on lines
+167
to
+170
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve number validation consistency. The undefined check is inconsistent with other input types and might miss invalid cases. Consider using the same validation pattern as suggested in past reviews. - (numberValue: number): void => {
- if (numberValue === undefined) return;
+ (numberValue: number | undefined): void => {
+ if (!Number.isFinite(numberValue)) return;
onChange(numberValue); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||
[onChange], | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
return ( | ||||||||||||||||||
<StudioInputTable.Cell.Numberfield | ||||||||||||||||||
ref={ref} | ||||||||||||||||||
aria-label={label} | ||||||||||||||||||
autoComplete={autoComplete} | ||||||||||||||||||
className={classes.textfieldCell} | ||||||||||||||||||
onChange={handleNumberChange} | ||||||||||||||||||
onFocus={onFocus} | ||||||||||||||||||
value={value as number} | ||||||||||||||||||
/> | ||||||||||||||||||
); | ||||||||||||||||||
}, | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
NumberfieldCell.displayName = 'NumberfieldCell'; | ||||||||||||||||||
|
||||||||||||||||||
const CheckboxCell = forwardRef<HTMLInputElement, TypedInputCellProps>( | ||||||||||||||||||
({ label, value, onChange, onFocus }, ref) => { | ||||||||||||||||||
const handleBooleanChange = useCallback( | ||||||||||||||||||
(event: React.ChangeEvent<HTMLInputElement>): void => { | ||||||||||||||||||
onChange(event.target.checked); | ||||||||||||||||||
}, | ||||||||||||||||||
[onChange], | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
return ( | ||||||||||||||||||
<StudioInputTable.Cell.Checkbox | ||||||||||||||||||
ref={ref} | ||||||||||||||||||
aria-label={label} | ||||||||||||||||||
onChange={handleBooleanChange} | ||||||||||||||||||
onFocus={onFocus} | ||||||||||||||||||
checked={value as boolean} | ||||||||||||||||||
value={String(value)} | ||||||||||||||||||
> | ||||||||||||||||||
{String(value)} | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are displaying this value in the value-column, maybe it should be translated? 🙈 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you suggest using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are translating these in the expression-tool at least. So using the same translation would provide consistency 🤷 |
||||||||||||||||||
</StudioInputTable.Cell.Checkbox> | ||||||||||||||||||
); | ||||||||||||||||||
}, | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
CheckboxCell.displayName = 'CheckboxCell'; | ||||||||||||||||||
|
||||||||||||||||||
const TextfieldCell = forwardRef<HTMLInputElement, TypedInputCellProps>( | ||||||||||||||||||
({ label, value, onChange, onFocus, autoComplete }, ref) => { | ||||||||||||||||||
const handleTextChange = useCallback( | ||||||||||||||||||
(event: React.ChangeEvent<HTMLInputElement>): void => { | ||||||||||||||||||
onChange(event.target.value); | ||||||||||||||||||
}, | ||||||||||||||||||
[onChange], | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
return ( | ||||||||||||||||||
<StudioInputTable.Cell.Textfield | ||||||||||||||||||
ref={ref} | ||||||||||||||||||
aria-label={label} | ||||||||||||||||||
autoComplete={autoComplete} | ||||||||||||||||||
className={classes.textfieldCell} | ||||||||||||||||||
onChange={handleTextChange} | ||||||||||||||||||
onFocus={onFocus} | ||||||||||||||||||
value={value as string} | ||||||||||||||||||
/> | ||||||||||||||||||
); | ||||||||||||||||||
}, | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
TextfieldCell.displayName = 'TextfieldCell'; | ||||||||||||||||||
|
||||||||||||||||||
type TextResourceIdCellProps = { | ||||||||||||||||||
currentId: string; | ||||||||||||||||||
label: string; | ||||||||||||||||||
|
@@ -159,7 +253,7 @@ function TextResourceIdCell(props: TextResourceIdCellProps): ReactElement { | |||||||||||||||||
if (textResources) { | ||||||||||||||||||
return <TextResourceSelectorCell {...props} textResources={textResources} />; | ||||||||||||||||||
} else { | ||||||||||||||||||
return <TextfieldCell label={label} onChange={onChangeCurrentId} value={currentId || ''} />; | ||||||||||||||||||
return <TypedInputCell label={label} onChange={onChangeCurrentId} value={currentId || ''} />; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it correct that the value should default to an empty string if no currentId exists? Maybe not a part of the scope of this PR tho, since creating initial typed entries in the codeList is not in the scope 🙈 |
||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import type { CodeListItem } from '../types/CodeListItem'; | ||
import type { CodeList } from '../types/CodeList'; | ||
|
||
const item1: CodeListItem = { | ||
description: 'Test 1 description', | ||
helpText: 'Test 1 help text', | ||
label: 'Test 1', | ||
value: true, | ||
}; | ||
|
||
const item2: CodeListItem = { | ||
description: 'Test 2 description', | ||
helpText: 'Test 2 help text', | ||
label: 'Test 2', | ||
value: false, | ||
}; | ||
|
||
export const codeListWithBooleanValues: CodeList = [item1, item2]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import type { CodeListItem } from '../types/CodeListItem'; | ||
import type { CodeList } from '../types/CodeList'; | ||
|
||
const item1: CodeListItem = { | ||
description: 'Positive number', | ||
helpText: 'Test 1 help text', | ||
label: 'Test 1', | ||
value: 1, | ||
}; | ||
|
||
const item2: CodeListItem = { | ||
description: 'Decimal', | ||
helpText: 'Test 2 help text', | ||
label: 'Test 2', | ||
value: 3.14, | ||
}; | ||
|
||
const item3: CodeListItem = { | ||
description: 'Negative number', | ||
helpText: 'Test 3 help text', | ||
label: 'Test 3', | ||
value: -1, | ||
}; | ||
|
||
export const codeListWithNumberValues: CodeList = [item1, item2, item3]; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be named |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
export type TypeofResult = | ||
| 'string' | ||
| 'number' | ||
| 'bigint' | ||
| 'boolean' | ||
| 'symbol' | ||
| 'undefined' | ||
| 'object' | ||
| 'function'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the onBlur be called at all in these cases? 🤔 Should not the
isCodeListValid
function catch tese issues?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the validation only checks if there are duplicate items:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agree, but I meant that we should adapt the validator, so it actually does the necessary checks so we wont need to call onBlur if there is no changes 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will take a look into it!