Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

feat: add Select component #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add Select component #8

wants to merge 1 commit into from

Conversation

MikaelSiidorow
Copy link
Collaborator

No description provided.

@MikaelSiidorow MikaelSiidorow force-pushed the feat/select branch 3 times, most recently from efa647c to 8fbea6e Compare April 15, 2023 16:33
@MikaelSiidorow MikaelSiidorow force-pushed the feat/select branch 2 times, most recently from 3293594 to f0676d1 Compare May 25, 2023 19:08
@MikaelSiidorow MikaelSiidorow changed the title Add Select component feat: add Select component May 25, 2023
@MikaelSiidorow MikaelSiidorow marked this pull request as ready for review May 25, 2023 19:09
Copy link

@WatMan77 WatMan77 left a comment

Choose a reason for hiding this comment

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

Seems like a lot of work for just a single component, but it looks fine.

WatMan77
WatMan77 previously approved these changes May 25, 2023
Copy link
Member

@PurkkaKoodari PurkkaKoodari left a comment

Choose a reason for hiding this comment

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

Seems to work - a couple potential hiccups:

  • Scrolling on mobile is a bit weird when reaching the end of the list
  • Not sure if this is just Storybook's fault, but stuff is showing through the select:

image

@MikaelSiidorow
Copy link
Collaborator Author

Seems to work - a couple potential hiccups:

  • Scrolling on mobile is a bit weird when reaching the end of the list
  • Not sure if this is just Storybook's fault, but stuff is showing through the select:

image

  • Increased z-index to avoid clipping
  • Increased vertical padding to make touch target larger

Other than small touch target, I don't see what's wrong on mobile 🤔 Can you clarify a bit more?

@PurkkaKoodari
Copy link
Member

PurkkaKoodari commented Jul 23, 2023

Trying to scroll past the top of the list (sometimes also bottom) selects the item where you start the scroll touch. Also some scroll glitchiness visible on the video.

az_recorder_20230723_125858.mp4

@MikaelSiidorow
Copy link
Collaborator Author

MikaelSiidorow commented Jul 24, 2023

Trying to scroll past the top of the list (sometimes also bottom) selects the item where you start the scroll touch. Also some scroll glitchiness visible on the video.

az_recorder_20230723_125858.mp4

Hmm, this bugginess seems to be present in the original radix-ui component as well. Perhaps we could look for an alternative Select implementation 🤔

This is not a big issue though, when the Select has a more reasonable amount of options. The example is maybe a bit exaggerated 😅

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

Successfully merging this pull request may close these issues.

3 participants