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.

Sign up for GitHub

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

Jump to bottom

Layout is messed up with RTL devices #38

Closed
dyaacov opened this issue Feb 17, 2017 · 35 comments
Closed

Layout is messed up with RTL devices #38

dyaacov opened this issue Feb 17, 2017 · 35 comments

Comments

@dyaacov
Copy link

dyaacov commented Feb 17, 2017

The example from google play doesn't work on LG G2
@bd-arc
Copy link
Contributor

bd-arc commented Feb 21, 2017

@dyaacov Hi! Thank you for letting us now. Could you be a bit more specific? Is the app crashing at startup? Are some parts of the app unavailable? Is it another kind of issue?

@dyaacov
Copy link
Author

dyaacov commented Mar 10, 2017 via email

@bd-arc
Copy link
Contributor

bd-arc commented Mar 10, 2017

@dyaacov Thank you for getting back to me!
It seems your screenshots didn't make it to GitHub ;) Would you mind hosting them online (on imgur for example)?

@dyaacov
Copy link
Author

dyaacov commented Mar 10, 2017 via email

@bd-arc
Copy link
Contributor

bd-arc commented Mar 13, 2017

@dyaacov Thank you for taking the time to upload these screenshots. Now begins the painful journey of trying to reproduce the bug on our devices ;)

@dyaacov
Copy link
Author

dyaacov commented Mar 13, 2017 via email

@bd-arc
Copy link
Contributor

bd-arc commented Mar 13, 2017

@dyaacov We've tried the app on a LG G3 and weren't able to reproduce the issue. I fear a buggy React Native's ScrollView implementation on some Android versions :-( That's why we're currently considering implementing our own PanResponder (see #40), but this requires major developments.

Can you share the Android versions on which you've tried the app, and specify the ones that were buggy?

@dyaacov
Copy link
Author

dyaacov commented Mar 13, 2017 via email

@bd-arc
Copy link
Contributor

bd-arc commented Mar 16, 2017

@dyaacov Just a quick update: I wasn't able to reproduce the issue, either on a simulator or a real device. Still investigating though ;)

@dyaacov
Copy link
Author

dyaacov commented Mar 16, 2017 via email

@bd-arc
Copy link
Contributor

bd-arc commented Mar 16, 2017

@dyaacov I've tried on the only one that was around, a LG G3, without seeing any issue. My guess is that there is something wrong with ScrollView implementation on some specific Android devices, but I need to be able to reproduce the bug to investigate further.

I'll look around for other LG devices.

@dyaacov
Copy link
Author

dyaacov commented Mar 16, 2017 via email

@bd-arc
Copy link
Contributor

bd-arc commented Mar 16, 2017

@dyaacov That's an idea, thanks! I'll prepare a debug apk as soon as I can (not before a few days I'm afraid).

In the meantime, and if this is not too much trouble, a short video of the issue might help figuring out what's happening.

@bd-arc bd-arc mentioned this issue Mar 22, 2017
Closed
@bd-arc
Copy link
Contributor

bd-arc commented Mar 24, 2017

@dyaacov Would you mind building the latest version of the example and tell me if you can still reproduce the issue on your devices?

I've made a few bug fixes in the latest release and also bumped the RN version of the example to the latest stable one; that might have helped ;)

@dyaacov
Copy link
Author

dyaacov commented Mar 26, 2017 •
edited
Loading

Sorry, same issues. when you first open it, the first slide is adjusted to left and when you try to swipe everything disappears.

See video:
https://drive.google.com/open?id=0B0KCjjvaqpcsaDlXSXF0Zm04Rm8

@bd-arc
Copy link
Contributor

bd-arc commented Mar 27, 2017 •
edited
Loading

@dyaacov Thank you very much for the video! Thanks to it, I've finally understood the root of the issue. This has to do with right-to-left UI. If you change your settings to left-to-right, you'll see the issue vanish.

You can see that items are rendered left-to-right (that's the default behavior of RN's ScrollView with RTL languages) but that our snap calculations are applied right-to-left. I'll look into a fix as soon as possible.

@yaronlevi
Copy link

It is a known issue. ScrollView + RTL = bug.
facebook/react-native#10852

@bd-arc bd-arc changed the title Doesn't work on LG G2 Layout is messed up with RTL devices Mar 29, 2017
@bd-arc
Copy link
Contributor

bd-arc commented Mar 29, 2017

@dyaacov @yaronlevi Good news! I've just published a new version of the plugin that adds RTL support and should resolve your issue.

As @yaronlevi stated, the issue had to do with React Native's RTL handling. I had to tweak a few things, so make sure to read the notes about this feature.

Can you both confirm that this solves the issue for you?

@dyaacov
Copy link
Author

dyaacov commented Mar 29, 2017

Hi,
I still see the same issue as in my previous video.
I verified I got the latest Git changes.

:(

@bd-arc
Copy link
Contributor

bd-arc commented Mar 29, 2017

@dyaacov This is strange...

Two days ago, I was finally able to reproduce the issue with I18nManager.forceRTL(true), and I've worked around it thanks to some heavy head-scratching.

I'm positive that this works since I've tested the fix on iOS and Android, on both emulators and real devices. Anyway, I'll triple-check tomorrow.

In the meantime, can you locally change this line to "react-native-snap-carousel": "2.1.0", run npm install in the /example folder and rebuild the example app? I myself had issues with npm's cache earlier and this helped.

@dyaacov
Copy link
Author

dyaacov commented Mar 30, 2017

YES!!!

  1. I changed to "react-native-snap-carousel": "2.1.0"
  2. removed node_modules and ran npm install again

Well done!
I'll do more testing in the next 2 days.

@bd-arc
Copy link
Contributor

bd-arc commented Mar 30, 2017

@dyaacov Good to hear! I'm closing the issue, but do not hesitate to keep me posted with the results of your tests ;)

@bd-arc bd-arc closed this as completed Mar 30, 2017
@dyaacov
Copy link
Author

dyaacov commented Mar 31, 2017

Found another bug in RTL only.
First time you load the carousel, the first image is not in focus (gray color and same size as the other images)

@bd-arc
Copy link
Contributor

bd-arc commented Apr 3, 2017

@dyaacov I have faced this issue while implementing RTL support, but I got rid of it and haven't been able to reproduce it since then.

Are you talking about the first slide to the left or to the right?

@bd-arc bd-arc reopened this Apr 3, 2017
@dyaacov
Copy link
Author

dyaacov commented Apr 3, 2017 via email

@bd-arc
Copy link
Contributor

bd-arc commented Apr 3, 2017

@dyaacov The reversed order is by design. I was assuming that, if you have an array of [1, 2, 3], you would expect slides to be rendered [3, 2, 1] so that they can be read from right to left in the same order as they would in a LTR layout. Was that a mistake?

@dyaacov
Copy link
Author

dyaacov commented Apr 3, 2017 via email

@dyaacov
Copy link
Author

dyaacov commented Apr 3, 2017 via email

@bd-arc
Copy link
Contributor

bd-arc commented Apr 3, 2017

@dyaacov Well, I'm not used to RTL interfaces but I'm trying to foresee what would seem more "natural" to every user of this plugin. My understanding was that everything was reversed between LTR and RTL layouts.

You mentioned slides with timestamp. In a LTR layout, I would render the following ('X' marks the first active slide):

      X 
[1st of March] [1st of April] [1st of May]

Shouldn't it be the following with a RTL layout?

                                 X 
[1st of May] [1st of April] [1st of March]

@bd-arc
Copy link
Contributor

bd-arc commented Apr 8, 2017

Closing as no further feedback was provided.
Feel free to continue discussion if needed.

@bd-arc bd-arc closed this as completed Apr 8, 2017
@malashkevich
Copy link

Hi @bd-arc
Big thanks for this lib man!
I want to reopen this one. I have the same issue as described on video above.
The bug appears only for android.
Seems like it's related to facebook/react-native#11960
I was able to 'fix' it by adding following contentContainerCustomStyle to carousel:

 Platform.OS === 'android' ? {flexDirection: I18nManager.isRTL ? 'row-reverse' : 'row'} : {};

Probably you can take a look on this?

lib version: 3.2.3
rn: 0.47

@bd-arc
Copy link
Contributor

bd-arc commented Sep 26, 2017

Hi @malashkevich,

I'm quite surprised because if you take a look at line 765 of the source code, you'll see that I'm already using such a fix (along with a few others that you can find in this commit)...

Would you mind testing with RN 0.48 and letting me know the outcome?

@bd-arc
Copy link
Contributor

bd-arc commented Sep 28, 2017

@malashkevich Also, can you try the provided example and let me know if everything is properly displayed for you?

It was working properly on the miscellaneous RTL devices I've tested it on, but I might have overlooked something...

@amitassaraf
Copy link

I am just going to add the it took me a while to figure out this is related to RTL. So for SEO reasons Im gonna add that if you encounter flickering or snap flickering in the carousel then it is related to this issue. The solution by @malashkevich works.

@ducpt2
Copy link

ducpt2 commented Jun 19, 2019

i using "react-native-snap-carousel": "3.8.0" and the issue still appear, i am using @malashkevich solution to fix it. Maybe i miss something? Any help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@malashkevich @dyaacov @yaronlevi @bd-arc @amitassaraf @ducpt2