Age Range Field #866
Age Range Field #866
Conversation
| @@ -0,0 +1,15 @@ | |||
| import { AgeRangeUnitEnum, UNIT_OPTIONS } from './model' | |||
|
|
|||
| export function getAvaibleAgeRangeUnits(excludedAgeRangeUnits: AgeRangeUnitEnum[]): AgeRangeUnitEnum[] { | |||
There was a problem hiding this comment.
Shouldn't there be test for this?
| import { LocaleConfiguration } from '../../i18n' | ||
| import { AgeRangeUnitEnum } from './model' | ||
|
|
||
| export function convertUnitAgeRangeEnumToLocaleText( |
There was a problem hiding this comment.
Shouldn't there be test for this?
| }) | ||
| }) | ||
|
|
||
| it('should show only avaible unit options when the button is clicked', async () => { |
| end?: string | ||
| } | ||
|
|
||
| export interface AgeRangeInputProps { |
There was a problem hiding this comment.
Do you really need to declare all these properties?
I believe you can work with something like InputProps or React.InputHTMLAttributes<HTMLInputElement>.
| const handleClickUnitOptionsButton = () => setOpen((state) => !state) | ||
|
|
||
| const handleChangeStart = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| let start: number = null |
There was a problem hiding this comment.
| let start: number = null | |
| let start: number = value?.start |
Then the else below isn't needed
|
|
||
| export function convertUnitAgeRangeEnumToLocaleText( | ||
| ageRangeUnitEnum: AgeRangeUnitEnum, | ||
| locale: LocaleConfiguration |
There was a problem hiding this comment.
| locale: LocaleConfiguration | |
| { ageRange }: LocaleConfiguration |
| unitOptions = unitOptions.filter((unit) => !excludedAgeRangeUnits.includes(unit)) | ||
| } | ||
|
|
||
| if (unitOptions.length === 0) { |
There was a problem hiding this comment.
Isn't there an isEmpty function in the project?
| describe('Unit dropdown', () => { | ||
| it('should show all unit options when the button is clicked', async () => { | ||
| const { getByTestId } = render(<AgeRangeField />) | ||
| const button = getByTestId('age-range-unit-button') |
There was a problem hiding this comment.
You could create a constant for this test-id, like the other two
| jest.useRealTimers() | ||
| }) | ||
|
|
||
| it("should call 'onFocus' and 'onBlur' when start input gets focus and it loses focus", async () => { |
There was a problem hiding this comment.
| it("should call 'onFocus' and 'onBlur' when start input gets focus and it loses focus", async () => { | |
| it("should call 'onFocus' and 'onBlur' when start input gets focus and then loses it", async () => { |
| export enum AgeRangeUnitEnum { | ||
| YEARS, | ||
| MONTHS, | ||
| DAYS, | ||
| } |
There was a problem hiding this comment.
will it be possible to add new units when using the component?
asking because I've seen cases where it could be used with weeks, for example
| export interface AgeRangeFieldProps extends AgeRangeInputProps, UseFormControlProps {} | ||
|
|
||
| export function AgeRangeField(props: AgeRangeFieldProps) { | ||
| const { label, error, ...rest } = props |
There was a problem hiding this comment.
where are label and error getting used? is it necessary to destructure it?
There was a problem hiding this comment.
Probably to avoid propagating these props to the AgeRangeInput
| let start: number = null | ||
|
|
||
| const startText = e.currentTarget?.value | ||
| const startParsed = parseInt(startText) | ||
|
|
||
| if (!isNaN(startParsed)) { | ||
| start = startParsed | ||
| } else if (!isEmpty(startText)) { | ||
| start = value?.start | ||
| } |
There was a problem hiding this comment.
| let start: number = null | |
| const startText = e.currentTarget?.value | |
| const startParsed = parseInt(startText) | |
| if (!isNaN(startParsed)) { | |
| start = startParsed | |
| } else if (!isEmpty(startText)) { | |
| start = value?.start | |
| } | |
| const start = parseInt(e.currentTarget?.value) || value?.start |
Are the isNaN and isEmpty checks really necessary in this case?
| const handleClose = () => { | ||
| setOpen(false) | ||
| anchorRef.focus() | ||
| } |
There was a problem hiding this comment.
Would using a useCallback be beneficial here?
| anchorRef.focus() | ||
| } | ||
|
|
||
| const avaibleUnitOptions = getAvaibleAgeRangeUnits(unitOptionsToExclude) |
There was a problem hiding this comment.
Would using a useMemo be beneficial here?
| switch (ageRangeUnitEnum) { | ||
| case AgeRangeUnitEnum.YEARS: | ||
| return locale.ageRange.years |
| start, | ||
| end: value?.end, | ||
| unit: value?.unit, |
There was a problem hiding this comment.
| start, | |
| end: value?.end, | |
| unit: value?.unit, | |
| ...value, | |
| start |
| export interface AgeRangeFieldProps extends AgeRangeInputProps, UseFormControlProps {} | ||
|
|
||
| export function AgeRangeField(props: AgeRangeFieldProps) { | ||
| const { label, error, ...rest } = props |
There was a problem hiding this comment.
Probably to avoid propagating these props to the AgeRangeInput
closes #865