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

Fix bug on slider when dragging in rtl mode #457

Merged
merged 3 commits into from Sep 22, 2023

Conversation

roberttaylor426
Copy link
Contributor

@roberttaylor426 roberttaylor426 commented Jul 18, 2023

What:

Fix bug on slider when dragging in rtl mode. If you drag to the left, the slider animates to the right (and vice-versa). This broken behaviour can be observed on the examples page.

See also here and here.

Why:

Dragging on the carousel is broken for rtl pages.

How:

Rather than require a prop to be passed into the component, the Slider component detects whether the direction is rtl and inverts deltaX during drag accordingly.

Checklist:

  • Documentation added/updated (N/A)
  • Typescript definitions updated (N/A)
  • Tests added and passing (Experiment page updated and verified - do we need automated tests for this behaviour?)
  • Ready to be merged

@Conor-Hinchee
Copy link
Collaborator

Thanks for the PR!

So instead of passing a prop users will now need to wrap the entire Carousel Provider in a div with dir=RTL ?

@roberttaylor426
Copy link
Contributor Author

roberttaylor426 commented Jul 18, 2023

Typically (99+% of cases?) I would expect dir to be set at the root html level. It's usually the whole page that's rtl, rather than a subset of it.

See here for example.

@roberttaylor426
Copy link
Contributor Author

@Conor-Hinchee I'm happy to take your lead on a couple of points here.

It's possible the code doesn't need to be quite as defensive as it now is (I added the guards in the face of failing tests and runtime errors).

It's also possible there may be a better place to perform this rtl check.

What are your thoughts?

@roberttaylor426
Copy link
Contributor Author

Hi @Conor-Hinchee, thanks for your work on this project.

Are you able to provide a rough timeline of when this PR might be reviewed, merged and released?

@Conor-Hinchee Conor-Hinchee merged commit 0d8799c into express-labs:master Sep 22, 2023
1 check passed
@tsi
Copy link

tsi commented Oct 16, 2023

Hi guys, thank you for your work on this! Is this really fixed? it's seem to slide the wrong way on both the rtl example as well as on my local testing.

@tsi
Copy link

tsi commented Oct 16, 2023

Actually, this comment was helpful.
Setting transform: scaleX(1); on the slider element fixes the swipe direction.

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.

Slider draging does not work in RTL direction
3 participants