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

Category indicator not displaying correctly when horizontally scrollable #206

Open
huynguyen93 opened this issue Aug 9, 2021 · 8 comments
Labels
bug Something isn't working

Comments

@huynguyen93
Copy link

huynguyen93 commented Aug 9, 2021

Hello,

First of all, thanks for your work with this project, it's great!

I just found a problem with the category indicator, it doesn't display well in 2 cases:

  • when the container size is smaller than 400px
  • when the document direction is RTL (ie: when language is Hebrew)

I attach 2 short videos so it's easier to see

Screencast.2021-08-09.16.31.43.mp4
Screencast.2021-08-09.16.33.20.mp4

I'm sorry, I don't have enough time to look into the code to suggest a fix...

@huynguyen93
Copy link
Author

From what I checked quickly, maybe we should do CSS border bottom for the category button instead of manual calculation?

@nolanlawson
Copy link
Owner

Thanks for reporting. For the RTL issue, I think it might be a separate issue, but for the "too small" issue, I had kind of assumed that the picker would never need to be scrolled horizontally like that. Although I suppose there are some valid use cases for that (small feature phones?).

The manual calculation is done for performance reasons, but maybe Chrome no longer has the perf issue described here so we can roll it back. I'll look into it.

// TODO: Chrome has an unfortunate bug where we can't use a simple percent-based transform
// here, becuause it's janky. You can especially see this on a Nexus 5.
// So we calculate of the indicator and use exact pixel values in the animation instead
// (where ResizeObserver is supported).
$: {
// eslint-disable-next-line no-unused-vars
indicatorStyle = `transform: translateX(${
widthCalculator.resizeObserverSupported
? `${currentGroupIndex * computedIndicatorWidth}px` // exact pixels
: `${currentGroupIndex * 100}%` // fallback to percent-based
})`
}

@nolanlawson nolanlawson added the bug Something isn't working label Aug 13, 2021
@huynguyen93
Copy link
Author

huynguyen93 commented Aug 13, 2021

Yep, I was testing with an iphone 6s, which has the width of only 375px including some padding/margin (I'm making a widget that can be used in different places).
My current workaround is to make it min-width 400px and show a horizontal scroll but I guess it's better to have a more responsive display...

Thanks for looking into it!

@nolanlawson
Copy link
Owner

nolanlawson commented Aug 13, 2021

The solution I intended for small screens was to change --num-columns and/or --category-emoji-size. E.g. you could do something like:

@media screen and (max-width: 375px) {
  emoji-picker {
    --num-columns: 6;
    --category-emoji-size: 1.125rem;
  }
}

Screenshot from 2021-08-13 10-00-05

I didn't really intend for it to ever be horizontally scrollable, but I guess I didn't do a good job explaining that in the docs. 😅 Or maybe I should just include some built-in CSS for small screens.

@nolanlawson
Copy link
Owner

OK, looks like the manual calculation (exact pixels vs percent-based) has nothing to do with this. There are two separate bugs:

  • when horizontal scrollbar is visible, indicator is wrong
  • when in RTL mode, indicator is wrong

BTW the reason I don't use a border-bottom is because there's no way to animate it performantly (GPU-accelerated).

@nolanlawson nolanlawson changed the title Category indicator not displaying correctly Category indicator not displaying correctly when horizontally scrollable Aug 13, 2021
@nolanlawson
Copy link
Owner

Opened a new issue for RTL: #212

@huynguyen93
Copy link
Author

Thanks, just my thought, we don't really need the "slide" animation, a simple border bottom works well IMO (it clearly indicates that we're in that category, and doesn't look too bad)

@nolanlawson
Copy link
Owner

I added some docs around this: #215

I don't plan to switch to border-bottom because I like the slide animation. Also I don't believe the categories should be horizontally scrollable on mobile – I think it's poor design because it reduces the discoverability (especially with hidden scrollbars, the user may not realize it can scroll).

I would prefer for people to add their own CSS so that the categories are not horizontally scrollable, but in theory it should be possible to make the indicator adapt to the width of the categories, so I'll leave this bug open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants