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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RTL issue in payload #733

Merged
merged 2 commits into from Dec 8, 2021
Merged

Fix RTL issue in payload #733

merged 2 commits into from Dec 8, 2021

Conversation

mreram
Copy link
Contributor

@mreram mreram commented Dec 4, 2021

All TextView(s) must be LTR because in RTL language the payload will be affected and we have some problems

馃摲 Screenshots

Screen Shot 2021-12-04 at 11 02 34 AM

馃搫 Context

When our app is RTL mode we have some problems with payload texts.

馃摑 Changes

I added force LTR text direction in payload items.

It must be ltr because in RTL language the payload will be affected and we have some problems
@mreram mreram marked this pull request as ready for review December 4, 2021 07:34
@vbuberen
Copy link
Collaborator

vbuberen commented Dec 4, 2021

Thanks for you contribution!
The screenshot you provided is how the payload is supposed to work for RTL locales?

@mreram
Copy link
Contributor Author

mreram commented Dec 4, 2021

Thanks for your consideration.
The above screenshot is before fixing the issue.
After this change:
Screen Shot 2021-12-04 at 5 46 44 PM

@Drjacky
Copy link

Drjacky commented Dec 5, 2021

The line "title": 亘乇丕蹖 卮賲丕 is correct but, it'd be better to keep the whole alignment the same as before(LTR); Cause now it's weird, hard to read and RTL developers have used to read from LTR.

@MiSikora
Copy link
Contributor

MiSikora commented Dec 5, 2021

I think JSON preview is for searching, inspection, and visual validation, not for reading a text like news or messages. I prefer the layout after changes in this PR. But I'm not a native RTL user. Maybe we could make a poll to get feedback from our community?

@vbuberen
Copy link
Collaborator

vbuberen commented Dec 5, 2021

@mreram Thanks for the after screenshot.

@Drjacky Thanks for your input here.

@MiSikora Had the same idea yesterday, so tweeted a question about the change: https://twitter.com/vbuberen/status/1467238705081503749?t=nT30nBL-UkyWXwOKUSUD9A&s=09
So far received just 2 replies.

@Drjacky
Copy link

Drjacky commented Dec 5, 2021

@mreram Would you please try to set textAlignment="gravity" or textAlignment="viewStart" to check if you could have the whole text LTR and have the fix on line "title":亘乇丕蹖 卮賲丕.

@mreram
Copy link
Contributor Author

mreram commented Dec 5, 2021

I assume in json format or other programming languages it's rare to use other languages as a keyword.
Also, every Farsi character is 2bytes and it's not good to use Farsi in json format as a key.
Thanks for your suggestion @Drjacky I will test it.

@mreram
Copy link
Contributor Author

mreram commented Dec 5, 2021

@Drjacky with textAlignment="gravity"
Screen Shot 2021-12-05 at 10 00 52 PM

And textAlignment="viewStart" is same as before.
I think the only one that works is textDirection="ltr" and it will be fixed in most cases. Anyway, I've created a layout file with the same name and I'm using LTR in my app to fix this issue.

@cortinico
Copy link
Member

I prefer the layout after changes in this PR

Same here. Also not a native RTL so hard to make a call. I believe some RTL users are coding ltr (at least the one I know personally) so I'm assuming ltr is also fine for visualizing payloads like JSON/XML and others. +1 for merging this

@vbuberen
Copy link
Collaborator

vbuberen commented Dec 8, 2021

Ok, if everyone agree here let's merge the fix.
Let's see if we get feedback afterwards.

@mreram Thanks again for your contribution and making Chucker a better tool for everyone 馃殌

@vbuberen vbuberen merged commit 6253a43 into ChuckerTeam:develop Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants