Skip to content

fixing combobox to use useFormProps and related component refactors #2351

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.

Sign up for GitHub

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

Merged
ktabors merged 4 commits into from
Oct 25, 2021
Merged

fixing combobox to use useFormProps and related component refactors #2351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d525b6
fixing combobox to use useFormProps and related component refactors
ktabors Sep 17, 2021
47fdd60
Merge branch 'main' into combobox_useformprops
ktabors Sep 21, 2021
f0cda1c
adding props to make sure mobile combobox tray behaves correctly
ktabors Sep 23, 2021
76edf46
Merge branch 'main' into combobox_useformprops
LFDanLu Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/@react-spectrum/combobox/src/MobileComboBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,8 @@ function ComboBoxTray(props: ComboBoxTrayProps) {
isLoading={showLoading && loadingState === 'filtering'}
loadingIndicator={loadingState != null && loadingCircle}
validationState={validationState}
labelAlign="start"
labelPosition="top"
wrapperChildren={(state.inputValue !== '' || loadingState === 'filtering' || validationState != null) && !props.isReadOnly && clearButton}
UNSAFE_className={
classNames(
Expand Down
1 change: 1 addition & 0 deletions packages/@react-spectrum/label/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"@react-aria/i18n": "^3.3.2",
"@react-aria/label": "^3.2.0",
"@react-aria/utils": "^3.9.0",
"@react-spectrum/form": "^3.2.3",
"@react-spectrum/layout": "^3.2.1",
"@react-spectrum/utils": "^3.6.2",
"@react-types/label": "^3.5.0",
Expand Down
2 changes: 2 additions & 0 deletions packages/@react-spectrum/label/src/Field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ import labelStyles from '@adobe/spectrum-css-temp/components/fieldlabel/vars.css
import {mergeProps} from '@react-aria/utils';
import React, {RefObject} from 'react';
import {SpectrumFieldProps} from '@react-types/label';
import {useFormProps} from '@react-spectrum/form';

function Field(props: SpectrumFieldProps, ref: RefObject<HTMLElement>) {
props = useFormProps(props);
Comment thread
snowystinger marked this conversation as resolved.
let {
label,
labelPosition = 'top' as LabelPosition,
Expand Down
1 change: 0 additions & 1 deletion packages/@react-spectrum/numberfield/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
"@react-aria/numberfield": "^3.1.0",
"@react-aria/utils": "^3.9.0",
"@react-spectrum/button": "^3.6.0",
"@react-spectrum/form": "^3.2.3",
"@react-spectrum/label": "^3.4.0",
"@react-spectrum/textfield": "^3.2.0",
"@react-spectrum/utils": "^3.6.2",
Expand Down
2 changes: 0 additions & 2 deletions packages/@react-spectrum/numberfield/src/NumberField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {SpectrumNumberFieldProps} from '@react-types/numberfield';
import {StepButton} from './StepButton';
import stepperStyle from '@adobe/spectrum-css-temp/components/stepper/vars.css';
import {TextFieldBase} from '@react-spectrum/textfield';
import {useFormProps} from '@react-spectrum/form';
import {useHover} from '@react-aria/interactions';
import {useLocale} from '@react-aria/i18n';
import {useNumberField} from '@react-aria/numberfield';
Expand All @@ -30,7 +29,6 @@ import {useProvider, useProviderProps} from '@react-spectrum/provider';

function NumberField(props: SpectrumNumberFieldProps, ref: FocusableRef<HTMLElement>) {
props = useProviderProps(props);
props = useFormProps(props);
let provider = useProvider();
let {
isQuiet,
Expand Down
1 change: 0 additions & 1 deletion packages/@react-spectrum/picker/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
"@react-aria/select": "^3.5.0",
"@react-aria/utils": "^3.9.0",
"@react-spectrum/button": "^3.6.0",
"@react-spectrum/form": "^3.2.3",
"@react-spectrum/label": "^3.4.0",
"@react-spectrum/listbox": "^3.5.3",
"@react-spectrum/overlays": "^3.4.4",
Expand Down
2 changes: 0 additions & 2 deletions packages/@react-spectrum/picker/src/Picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,13 @@ import React, {ReactElement, useCallback, useRef, useState} from 'react';
import {SpectrumPickerProps} from '@react-types/select';
import styles from '@adobe/spectrum-css-temp/components/dropdown/vars.css';
import {Text} from '@react-spectrum/text';
import {useFormProps} from '@react-spectrum/form';
import {useMessageFormatter} from '@react-aria/i18n';
import {useProvider, useProviderProps} from '@react-spectrum/provider';
import {useSelectState} from '@react-stately/select';

function Picker<T extends object>(props: SpectrumPickerProps<T>, ref: DOMRef<HTMLDivElement>) {
props = useSlotProps(props, 'picker');
props = useProviderProps(props);
props = useFormProps(props);
let formatMessage = useMessageFormatter(intlMessages);
let {
isDisabled,
Expand Down
1 change: 0 additions & 1 deletion packages/@react-spectrum/searchwithin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
"@react-aria/label": "^3.1.3",
"@react-aria/utils": "^3.8.2",
"@react-spectrum/label": "^3.3.4",
"@react-spectrum/form": "^3.2.3",
"@react-spectrum/utils": "^3.6.2",
"@react-types/searchwithin": "3.0.0-alpha.0",
"@react-types/shared": "^3.8.0"
Expand Down
2 changes: 0 additions & 2 deletions packages/@react-spectrum/searchwithin/src/SearchWithin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,11 @@ import {FocusableRef} from '@react-types/shared';
import React, {useCallback, useLayoutEffect, useRef, useState} from 'react';
import {SpectrumSearchWithinProps} from '@react-types/searchwithin';
import styles from '@adobe/spectrum-css-temp/components/searchwithin/vars.css';
import {useFormProps} from '@react-spectrum/form';
import {useLabel} from '@react-aria/label';
import {useProvider, useProviderProps} from '@react-spectrum/provider';

function SearchWithin(props: SpectrumSearchWithinProps, ref: FocusableRef<HTMLElement>) {
props = useProviderProps(props);
props = useFormProps(props);
let {styleProps} = useStyleProps(props);
let {labelProps, fieldProps} = useLabel(props);
let {
Expand Down
1 change: 0 additions & 1 deletion packages/@react-spectrum/textfield/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
"@react-aria/interactions": "^3.6.0",
"@react-aria/textfield": "^3.4.0",
"@react-aria/utils": "^3.9.0",
"@react-spectrum/form": "^3.2.3",
"@react-spectrum/label": "^3.4.0",
"@react-spectrum/utils": "^3.6.2",
"@react-stately/utils": "^3.2.2",
Expand Down
4 changes: 0 additions & 4 deletions packages/@react-spectrum/textfield/src/TextFieldBase.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ import {PressEvents} from '@react-types/shared';
import React, {cloneElement, forwardRef, HTMLAttributes, InputHTMLAttributes, LabelHTMLAttributes, ReactElement, Ref, RefObject, TextareaHTMLAttributes, useImperativeHandle, useRef} from 'react';
import {SpectrumTextFieldProps, TextFieldRef} from '@react-types/textfield';
import styles from '@adobe/spectrum-css-temp/components/textfield/vars.css';
import {useFormProps} from '@react-spectrum/form';
import {useHover} from '@react-aria/interactions';
import {useProviderProps} from '@react-spectrum/provider';

interface TextFieldBaseProps extends Omit<SpectrumTextFieldProps, 'onChange'>, PressEvents {
wrapperChildren?: ReactElement | ReactElement[],
Expand All @@ -39,8 +37,6 @@ interface TextFieldBaseProps extends Omit<SpectrumTextFieldProps, 'onChange'>, P
}

function TextFieldBase(props: TextFieldBaseProps, ref: Ref<TextFieldRef>) {
props = useProviderProps(props);
Copy link
Copy Markdown
Member

@snowystinger snowystinger Oct 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how'd we remove useProviderProps from this one? but not from SearchWithin?

Copy link
Copy Markdown
Collaborator Author

@ktabors ktabors Oct 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useProviderProps is useful for a component that is being rendered directly like SearchWithin. If it is a child like TextFieldBase which other components use as a child, this call doesn't do anything. And useProviderProps can't exist here and not in the parent. useProviderProps must be in the parent. It we had Storybook tests of TextFieldBase we would need this useProviderProps call.

Copy link
Copy Markdown
Member

@LFDanLu LFDanLu Oct 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO I think we are ok pulling useProviderProps from TextFieldBase, feels like calling that hook is something each parent component that uses TextFieldBase should handle (e.g. if a component needs to override one of the provider props).

Copy link
Copy Markdown
Member

@LFDanLu LFDanLu Oct 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, useProviderProps favors the provided props over the props from the Provider so perhaps it would make sense to keep useProviderProps in TextFieldBase and remove it from the parent components that use it?

Copy link
Copy Markdown
Collaborator Author

@ktabors ktabors Oct 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to remind myself, I move useProviderProps from TextField to TextFieldBase. I goto the form stories, things look good until I get to isDsiabled, TextField is not disabled. Same in the provider stories. This might be a bug with isDisabled.

Looking at SearchWithin it seems like it shouldn't need useProviderProps, but when I remove it isDisabled breaks (which is weird, because I'd expect SearchField to pick it up) and the required indicator stops working.

I think there is a bug. I'm debugging the code and found that calling useProviderProps({}) in SearchField returns

isDisabled: true
isEmphasized: undefined
isQuiet: undefined
isReadOnly: undefined
isRequired: undefined
validationState: undefined

but calling the props after props = useProviderProps(props); still have isDsiabled as undefined.

UNSAFE_className: "searchwithin_spectrum-SearchWithin-searchfield_9a8c2"
aria-label: undefined
aria-labelledby: "react-aria489618031-2628"
id: "react-aria489618031-2627"
isDisabled: undefined
isEmphasized: undefined
isQuiet: false
isReadOnly: undefined
isRequired: undefined
label: null
placeholder: "Search"
validationState: null

props = useFormProps(props);
let {
label,
validationState,
Expand Down