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

CustomSelectControl: explore ariakit refactor #55234

Closed
wants to merge 1 commit into from

Conversation

brookewp
Copy link
Contributor

@brookewp brookewp commented Oct 11, 2023

What?

WIP

Tracked in #55023

Why?

How?

  • Review existing features of CustomSelectControl
  • Create a rough component to start exploring API
  • Determine if we can replace our current component
  • Assess current tests for CustomSelectControl
  • Add tests to the new component (cover any bugs, new features, etc)
  • Refine types, add docs, and stories
  • Request design input and implement styles
  • Look at requests for new component
    • Render children for more flexibility and customization

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

@brookewp brookewp added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Oct 11, 2023
@brookewp brookewp self-assigned this Oct 11, 2023
@brookewp brookewp requested a review from ajitbohra as a code owner October 11, 2023 00:20
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To note, this is just temporary CSS added from the Ariakit example.

const { label, onChange, options } = props;

const store = Ariakit.useSelectStore( {
setValue: ( value ) => onChange?.( value ),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our current CustomSelectControl expects an object to be returned via the onChange event:

} = useSelect( {
initialSelectedItem: items[ 0 ],
items,
itemToString,
onSelectedItemChange,
...( typeof _selectedItem !== 'undefined' && _selectedItem !== null
? { selectedItem: _selectedItem }
: undefined ),

Which is managed by useSelect; part of the package downshift.

In Ariakit, setValue appears to be what we'd use in lieu of useSelect, but value/setValue expect a string (or an array of strings for multi-select).

Copy link
Member

Choose a reason for hiding this comment

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

It appears that the object passed to onSelectItemChange has the following structure:

{
  selectedItem: "Curium",
  type: "__item_click__",
  highlightedIndex: -1,
  isOpen: false,
  inputValue: "",
}

I'm not sure if all the elements in this structure are part of the public API. The type value, in particular, seems obscure. I'm also unsure about the meaning of highlightedIndex, as it always appears to be -1.

Regardless, everything except for type can be readily accessed from within the setValue callback. You can use store.getState() in this context as well:

const store = useSelectStore({
  async setValue(selectedItem) {
    if (!onChange) return;
    // Executes the logic in a microtask after the popup is closed. This might not be necessary.
    // This is simply to ensure the isOpen state matches that in Downshift.
    await Promise.resolve();
    const state = store.getState();
    const changeObject = {
      selectedItem,
      type: "__item_click__",
      highlightedIndex: state.renderedItems.findIndex((item) => item.value === selectedItem),
      isOpen: state.open,
      inputValue: "",
    };
    onChange(changeObject);
  },
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this! We finally have gotten a chance to try this out. 🙂

@brookewp
Copy link
Contributor Author

Going to close this as we have two more recent options for this: #57902 and #57000

@brookewp brookewp closed this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
Status: Abandoned ⛔️
Development

Successfully merging this pull request may close these issues.

2 participants