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

✨(react) add async mode to Select #305

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

daproclaima
Copy link
Collaborator

@daproclaima daproclaima commented Mar 15, 2024

Mono-searchable Select fetches options on search update

An uncontrolled and a controlled mono-searchable select component can now take a callback as an options prop to fetch data and format it into an array of options to display.

This PR does not enable the use of async mode on the multi select.

Related to issue 286.

Copy link

changeset-bot bot commented Mar 15, 2024

⚠️ No Changeset found

Latest commit: 13e89dc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@NathanVss NathanVss left a comment

Choose a reason for hiding this comment

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

Nice job, this component is not the easiest to apprehend at first, you did well but here are some various comment here and there

Copy link
Contributor

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

I couldn't test properly the component, I have a bug with it, I let you see.
Try maybe the component from a demo (app) perspective, testing on storybook is not the best because storybook is not using the code transpiled by Vite (Rollup).


You should add some tests on multi.spec.tsx as well.


These commits seems to be fixup commit, they should be squash with what they are correcting.
🏷️(react) update types for Select/multi
🚨(react) fix type errors for Select Mono
🚨(react) lint fix in Form/Select
🏷️(react) change options prop of SelectMonoSimple

packages/react/src/components/Forms/Select/index.tsx Outdated Show resolved Hide resolved
packages/react/src/components/Forms/Select/index.tsx Outdated Show resolved Hide resolved
packages/react/src/components/Forms/Select/index.tsx Outdated Show resolved Hide resolved
@daproclaima daproclaima force-pushed the feat/async-options-for-select branch 3 times, most recently from 7a9fb8d to 751a525 Compare April 8, 2024 14:18
@daproclaima daproclaima force-pushed the feat/async-options-for-select branch 7 times, most recently from 7794304 to f6ac1d7 Compare June 17, 2024 10:47
@daproclaima daproclaima force-pushed the feat/async-options-for-select branch 6 times, most recently from 4157989 to 1f1640c Compare June 20, 2024 23:57
@jbpenrath
Copy link
Member

Few UI/UX feedbacks :

  1. I think the loading state can be improved
    1. I'll display the loader at the place of the arrow to open the select
    2. I'll disable the select during loading to prevent the user to open the menu
Before After
image image
  1. By testing stories, I feel each time the select value change, the request is executed.
CleanShot.2024-06-21.at.09.59.42.mp4

Copy link
Member

Choose a reason for hiding this comment

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

You should also add stories for multi select

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes @jbpenrath, I'll take care of it once I have finished the work on the mono select.

packages/react/src/components/Forms/Select/test-utils.tsx Outdated Show resolved Hide resolved
@daproclaima daproclaima force-pushed the feat/async-options-for-select branch 2 times, most recently from 5c00f06 to 7223555 Compare June 24, 2024 10:00
@jbpenrath
Copy link
Member

Few UI/UX feedbacks :

  1. I think the loading state can be improved

    1. I'll display the loader at the place of the arrow to open the select
    2. I'll disable the select during loading to prevent the user to open the menu

Before After
image image
2. By testing stories, I feel each time the select value change, the request is executed.

CleanShot.2024-06-21.at.09.59.42.mp4

@daproclaima daproclaima force-pushed the feat/async-options-for-select branch from 81a13de to 16a078b Compare June 27, 2024 21:54
@daproclaima
Copy link
Collaborator Author

Hi @jbpenrath, @AntoLC. I updated the select mono searchable component according to your feedbacks Jean-Baptiste.

When the component uses options fetching, and we click on the menu toggle button, the menu closes but instantly opens back. I do not find which event opens the menu back. Do you guys have an idea for fixing it?

See video below:

menu_toggle.mov

@daproclaima daproclaima requested a review from jbpenrath July 8, 2024 09:26
Add *.tool-versions to .gitignore
Add a test asserting that a mono uncontrolled
searchable select can work with an async
callback given as an options prop.
Add types for the callback provided as an options
prop for the Form/Select component
Add Searchable Uncontrolled With Async Options
Fetching and Searchable Uncontrolled With Async
Options Fetching And Default Value stories in
storybook.
Change SelectMono so that only an array of
options is passed to SelectMonoSimple since
options prop of SelectMono may also be a
callback to pass to SelectMonoSearchable.

This new feature allow to pass an async callback as
options prop for a Searchable Mono Select
component. Give it a function to fetch
dynamically data from a third service and format
them into an array of options.
A context param is automatically passed to the
callback so that the function is able to filter
tha data according to the search string. If the
props.defaultValue is provided, then the Select
will pick a default option matching the default
value.
Prevents useless triggering of async options fetching due
to default value and inputFilter updates handling in
mono-searchable.ts.
updates Mono.spec tests according to improvement related with
async data fetching callback preventing useless execution.
Add style for the loader of Select component when it uses
async options callback fetching.
Update mono.spec tests to verify the use of the new isLoading and
default value, async options fetching props and the loader.
updates english and french translations for the loader
of select component showed during options fetching.
    Creates a vitest workspace configuration file in root folder.
    It allows to run all tests in debug mode inside IntelliJ
    - https://vitest.dev/guide/debugging.html\#intellij-idea
    - https://vitest.dev/guide/workspace.html\#defining-a-workspace
It simplifies the code related with the asynchronous options
fetching and renames related component tests names
This prepares the select component to work both in controlled
and uncontrolled context when the component is searchable and
has to fetch the options. Also refactor the related tests and
add a skipped test for controlled context.
Add component test for controlled Select mono searchable
with fetched options. Makes sure to not call the fetch
options callback when previous search is the same
as the current one.
Add story for searchable controlled with async options fetching
mono select.
It updates the storybook files by adding instructions about the select
mono searchable with options fetching.
Adds or removes loader and actions buttons according to loading status
when select mono searchable is fetching options.
Options fetching callback is now executed only on search input change event
and when component is mounted if it uses a default value.
Previously, even selecting an option used to trigger the options fetching.
The select also now has the disabled status when initial fetch options
callback is triggered.
Allows to clear selected options when select mono searchable
uses options fetching and is controlled.
Prevents options menu to open again at click on arrow down button
when select mono searchable uses options fetching
Allows to clear selected options when select mono searchable
uses options fetching and is controlled.
Stories with searchable and options fetching now
fetch all options without filtering them with
search term at initial options fetching
- fix displaying and hidding of inner actions elements
accordingly to loading state and apply display none on
action arrow button
- updates related component tests

The arrow button needs to remain in the dom to not
reset its react ref. That is why we apply a display
none css rule to its visual element instead of taking
the component out of the DOM.
- make arrow button keyboard navigable
- update related component tests
- apply fixes to SelectMonoSearchable concerning loading state
and refactors
- prevent passing a string array in value prop to select mono
- update storybook documentation and options fetching utils
- execute options fetching on select mono searchable when
clear button is clicked
- update related tests and a new one
- update description for searchable select with
options fetching
- simplify the reading of the condition
displaying the clear button
- update CHANGELOG.md for release
@daproclaima daproclaima force-pushed the feat/async-options-for-select branch from 9ef000c to fde34ca Compare September 24, 2024 06:42
- remove unused error var
@daproclaima daproclaima force-pushed the feat/async-options-for-select branch from fde34ca to 13e89dc Compare September 24, 2024 06:43
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

Successfully merging this pull request may close these issues.

4 participants