Skip to content

Conversation

@daproclaima
Copy link

@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.

@changeset-bot
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
Collaborator

@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
Collaborator

@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

@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
Collaborator

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
Collaborator

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
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.

@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
Collaborator

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
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
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
@daproclaima
Copy link
Author

@jbpenrath I see this PR is still open. I've some free time to finish the work and clean it up. I noticed that many things I added can be simplified

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.

5 participants