Skip to content
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

Create a consistent API for event handlers in v11 #9641

Closed
3 tasks
Tracked by #9535
joshblack opened this issue Sep 10, 2021 · 3 comments
Closed
3 tasks
Tracked by #9535

Create a consistent API for event handlers in v11 #9641

joshblack opened this issue Sep 10, 2021 · 3 comments

Comments

@joshblack
Copy link
Contributor

joshblack commented Sep 10, 2021

We would like any on* or event-handler-related props to have consistent API across the codebase. During an initial review, we noticed that several patterns are floating around the codebase that has been developed throughout the years:

  • (value) => void
  • (value, event) => void
  • (event) => void
  • (event, value) => void

Checklist

  • Conduct an initial audit of existing conventions for event handlers
  • Document our conventions that guide our project refactor
  • Refactor the codebase
@joshblack joshblack mentioned this issue Sep 10, 2021
20 tasks
@joshblack joshblack changed the title Event handler signatures Create consistent event handler signatures in v11 Sep 10, 2021
@joshblack

This comment has been minimized.

@joshblack joshblack changed the title Create consistent event handler signatures in v11 Create a consistent API for event handlers in v11 Sep 10, 2021
@joshblack
Copy link
Contributor Author

joshblack commented Sep 13, 2021

  • src/components/Accordion/AccordionItem.js
    • onHeadingClick({ isOpen: boolean; event: any; }) -> onChange({ isOpen: boolean })
  • src/components/Checkbox/Checkbox.js
    • onChange(ChangeEvent<HTMLInputElement>, { checked: boolean; id: any; })
    • onChange(boolean, any, ChangeEvent<HTMLInputElement>)
  • src/components/ListBox/ListBoxSelection.js
    • onClearSelection(any) -> onClearSelection()
  • src/components/ListBox/next/ListBoxSelection.js
    • onClearSelection(any) -> onClearSelection()
  • src/components/ComboBox/ComboBox.js
    • onChange({ selectedItem: any; })
    • onToggleClick(any)
    • handleToggleClick(boolean)
  • src/components/ComposedModal/ComposedModal.js
    • onClose(event) -> onClose()
  • src/components/DataTable/DataTable.js
    • onClick(any, any)
    • onClick(any, any)
  • src/components/Menu/MenuOption.js
    • onClick(any)
  • src/components/Menu/MenuRadioGroupOptions.js
    • onChange(any)
    • handleClick(any)
  • src/components/Menu/MenuSelectableItem.js
    • onChange(boolean)
  • src/components/Menu/Menu.js
    • onClose()
  • src/components/OverflowMenu/OverflowMenu.js
    • onClose()
    • onCloseMenu()
  • src/components/DataTable/TableToolbarSearch.js
    • onChangeProp("", any)
    • onExpand(any, boolean)
    • onChangeProp(any)
    • handleExpand(any, true)
    • handleExpand(any, false)
  • src/components/DatePickerInput/DatePickerInput.js
    • onChange(any)
    • onClick(any)
  • src/components/DatePicker/DatePicker.js
    • onChange(any)
    • onHook(any)
    • onOpen(any)
  • src/components/Dropdown/Dropdown.js
    • onChange({ selectedItem: any; })
  • src/components/FileUploader/FileUploaderButton.js
    • onChange(any)
  • src/components/FileUploader/FileUploaderDropContainer.js
    • onAddFiles(any, { addedFiles: any; })
    • handleChange(DragEvent<HTMLDivElement>)
  • src/components/FileUploader/FileUploaderItem.js
    • onDelete(any, { uuid: any; })
    • onDelete(any, { uuid: any; })
  • src/components/InlineCheckbox/InlineCheckbox.js
    • onChange(any, any, any)
  • src/components/InlineLoading/InlineLoading.js
    • onSuccess()
  • src/components/Notification/next/Notification.js
    • onClose(any)
    • onCloseButtonClick(any)
    • handleClose(any)
  • src/components/Notification/next/Notification.js
    • onClose(any)
    • onCloseButtonClick(any)
    • handleClose(any)
  • src/components/Notification/Notification.js
    • onClose(any)
    • onCloseButtonClick(any)
    • handleClose(any)
  • src/components/Notification/Notification.js
    • onClose(any)
    • onCloseButtonClick(any)
    • handleClose(any)
  • src/components/TextInput/ControlledPasswordInput.js
    • onChange(any)
    • onClick(any)
  • src/components/TextInput/PasswordInput.js
    • onTogglePasswordVisibility(any)
    • onChange(any)
    • onClick(any)
  • src/components/TextInput/TextInput.js
    • onChange(any)
    • onClick(any)
  • src/components/ModalWrapper/ModalWrapper.js
    • handleSubmit()
    • onKeyDown(KeyboardEvent<HTMLDivElement>)
  • src/components/MultiSelect/FilterableMultiSelect.js
    • onItemChange(any)
  • src/components/MultiSelect/MultiSelect.js
    • onMenuChange(any)
    • onItemChange(any)
  • src/components/NumberInput/NumberInput.js
    • onChange(any, { value: any; direction: string; })
    • onChange(any, { value: any; direction: string; })
    • onClick(any, { value: any; direction: any; })
    • onChange(any, { value: any; direction: any; })
    • onClick(any, any, any)
    • onChange(any, any, any)
  • src/components/OverflowMenuItem/OverflowMenuItem.js
    • onClick(any)
    • onKeyDown(any)
  • src/components/Pagination/experimental/Pagination.js
    • onChange({ page: any; pageSize: any; })
    • onChange({ page: number; pageSize: any; })
  • src/components/PaginationNav/PaginationNav.js
    • onSelect(number)
    • onSelect(any)
  • src/components/PaginationNav/PaginationNav.js
    • onChange(number) -> onChange({ index })
  • src/components/ProgressIndicator/ProgressIndicator.js
    • ProgressStep onClick()
    • ProgressIndicator onChange(any)
  • src/components/RadioTile/RadioTile.js
    • onChange(value, name, ChangeEvent) -> onChange(ChangeEvent)
  • src/components/SearchLayoutButton/SearchLayoutButton.js
    • onChangeFormat({ format: string; })
  • src/components/Switch/Switch.js
    • onClick({ index: any; name: any; text: any; })
    • onKeyDown({ index: any; name: any; text: any; key: any; })
  • src/components/Tab/Tab.js
    • handleTabClick(index, MouseEvent<HTMLLIElement, MouseEvent>)
    • onClick(MouseEvent<HTMLLIElement, MouseEvent>)
    • handleTabKeyDown(any, KeyboardEvent<HTMLLIElement>)
    • onKeyDown(KeyboardEvent<HTMLLIElement>)
  • src/components/Tabs/Tabs.js
    • onSelectionChange(any) -> onChange({ selectedIndex })
  • src/components/Tag/Tag.js
    • onClose(any) -> onClose(), onClick(event)
  • src/components/TextArea/TextArea.js
    • onChange(any)
    • onClick(event)
  • src/components/Tile/Tile.js
    • SelectableTile -> onChange(ClickEvent | KeyboardEvent | ChangeEvent), onChange({ isSelected: boolean })
  • src/components/TimePicker/TimePicker.js
    • onChange(any)
    • onClick(any)
    • onBlur(any)
  • src/components/Toggle/next/Toggle.js
    • onClick(event)
  • src/components/Toggle/Toggle.js
    • onChange(ChangeEvent<HTMLInputElement>)
    • onToggle(any, any, ChangeEvent<HTMLInputElement>)
  • src/components/TreeView/TreeNode.js
    • onToggle(any, { id: any; isExpanded: boolean; label: any; value: any; })
    • onTreeSelect(any, { id: any; label: any; value: any; })
    • onNodeSelect(any, { id: any; label: any; value: any; })
    • onNodeFocusEvent(any)
  • src/components/TreeView/TreeView.js
    • onSelect(any, {}) -> onChange(data)
  • src/components/UIShell/SideNav.js
    • onToggle(any, boolean)
    • handleToggle(any, true)
    • handleToggle(any, false)
    • handleToggle(true, true)
    • handleToggle(false, false)
  • src/components/UIShell/SideNavFooter.js
    • onToggle(MouseEvent<HTMLButtonElement, MouseEvent>) -> onToggle, plus onClick(event)

  • Form Components
    • TextArea

@joshblack
Copy link
Contributor Author

joshblack commented Sep 13, 2021

Proposal

Controlled components

// A change handler is an `onChange` prop for a controlled component
// This handler is generic over the data/state that is made public by the component.
// It should be of type object so that it is easy to modify/extend with additional values
// in the future
type ChangeHandler<T extends object> = (data: T) => void;

function Example() {
  const [value, setValue] = React.useState('');

  return (
    <Sample
      value={value}
      onChange={(data) => setValue(data.value)}
    />
  );
}

Controlled components with native onChange

In situations where a controlled component may have a native onChange event, we often combine the onChange event with the change handler defined above. This can have several types:

  • onChange(event, data)
  • onChange(data, event)
  • onChange({ event, value })
  • etc

We have several possible directions forward:

  • Keep onChange strictly as the native event, users can derive value from the event like we do
  • Have a hybrid onChange that standardizes a native event and a ChangeHandler
  • Change onChange to strictly be a ChangeHandler
  • Keep onChange as the native event and have a onValueChange as a ChangeHandler for this unique case that we have in the codebase (often for components for form elements)
  • Split up components into a "Base" and "Controlled" variant so that onChange is contextual. For Base it would be the native event (uncontrolled), for Controlled it would be a ChangeHandler (controlled)

The most compelling options seem to be:

  • Standardize on a hybrid onChange for these special moments (easy to migrate, transition)
  • Split up components in a Base and Controlled variant (hard to migrate, provides more API)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant