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 select options custom render #187

Merged
merged 2 commits into from Oct 19, 2023

Conversation

NathanVss
Copy link
Member

@NathanVss NathanVss commented Oct 5, 2023

Mono

Capture d’écran 2023-10-06 à 16 56 17

Multi

Capture d’écran 2023-10-06 à 16 56 31

@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2023

⚠️ No Changeset found

Latest commit: 913512e

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

@AntoLC AntoLC mentioned this pull request Oct 6, 2023
14 tasks
@NathanVss NathanVss changed the title ✨(react) select choice can be ReactNode (react) add select options custom render Oct 6, 2023
@NathanVss NathanVss changed the title (react) add select options custom render ✨(react) add select options custom render Oct 6, 2023
@NathanVss NathanVss force-pushed the feat/select-render-custom branch 2 times, most recently from 0eb3fd6 to 0f85b68 Compare October 6, 2023 15:10
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.

Looks good, meets Marsha's needs!

2 remarks:

  • height relative to the content
    scrnli_10_9_2023_10-52-08 AM
  • In searchable mode, when we click on the option, the filter stay active so we don't see the ReactNode until we click out of the select
scrnli_10_9_2023_10-53-40.AM.webm

@NathanVss
Copy link
Member Author

Looks good, meets Marsha's needs!

2 remarks:

  • height relative to the content
    scrnli_10_9_2023_10-52-08 AM
  • In searchable mode, when we click on the option, the filter stay active so we don't see the ReactNode until we click out of the select

scrnli_10_9_2023_10-53-40.AM.webm

I get what you mean, in reality it was 100% made on purpose to fit the design system. The select is not intended to be used with exotic heights, in such cases it probably means the UI/UX of the feature could be reworked in another way to be able to use native components based on quick talk with @jbpenrath

WDYT @AntoLC ?

@NathanVss NathanVss requested a review from AntoLC October 17, 2023 09:08
@AntoLC
Copy link
Contributor

AntoLC commented Oct 18, 2023

I get what you mean, in reality it was 100% made on purpose to fit the design system. The select is not intended to be used with exotic heights, in such cases it probably means the UI/UX of the feature could be reworked in another way to be able to use native components based on quick talk with @jbpenrath

WDYT @AntoLC ?

I think that the client would have about 15px to be able to render his option without modifying the height, so it will not cover lot of use cases.

image

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.

  • In searchable mode, when we click on the option, the filter stay active so we don't see the ReactNode until we click out of the select

scrnli_10_9_2023_10-53-40.AM.webm

What about this part?

@jbpenrath
Copy link
Member

Looks good, meets Marsha's needs!

2 remarks:

  • height relative to the content
    scrnli_10_9_2023_10-52-08 AM
  • In searchable mode, when we click on the option, the filter stay active so we don't see the ReactNode until we click out of the select

scrnli_10_9_2023_10-53-40.AM.webm

@AntoLC could you provide a video of the current implementation of this select ?

@AntoLC
Copy link
Contributor

AntoLC commented Oct 18, 2023

could you provide a video of the current implementation of this select ?

In our Marsha use cases we have that:
Capture d'écran 2023-10-18 113951
Capture d'écran 2023-10-18 114046

All are just observations, we can change the height on our side if it's complicated for you.

@NathanVss
Copy link
Member Author

  • In searchable mode, when we click on the option, the filter stay active so we don't see the ReactNode until we click out of the select

scrnli_10_9_2023_10-53-40.AM.webm

What about this part?

What other possibilities do we have? In search mode when giving focus to input we need to deal with text, so that's why we use label we can't fit the ReactNode inside a text input.

@AntoLC
Copy link
Contributor

AntoLC commented Oct 18, 2023

  • In searchable mode, when we click on the option, the filter stay active so we don't see the ReactNode until we click out of the select

scrnli_10_9_2023_10-53-40.AM.webm

What about this part?

What other possibilities do we have? In search mode when giving focus to input we need to deal with text, so that's why we use label we can't fit the ReactNode inside a text input.

Other possibility, when you click on the option you display the option, as it is now when you click on the option you still display the filter until the user click out of the select as shown in the video.

@AntoLC AntoLC self-requested a review October 18, 2023 16:23
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.

All good, fits Marsha cases.

@NathanVss
Copy link
Member Author

  • In searchable mode, when we click on the option, the filter stay active so we don't see the ReactNode until we click out of the select

scrnli_10_9_2023_10-53-40.AM.webm

What about this part?

What other possibilities do we have? In search mode when giving focus to input we need to deal with text, so that's why we use label we can't fit the ReactNode inside a text input.

Other possibility, when you click on the option you display the option, as it is now when you click on the option you still display the filter until the user click out of the select as shown in the video.

I understand, it would imply a structural change were when you select an option you loose focus on the search input, which is not what we see on other libraries at most 🤔

We want to be able to render the options in a customized manner.
We want to be able to render the options in a customized manner.
@NathanVss NathanVss merged commit b86ba5c into main Oct 19, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants