Skip to content

Fix RTL issue in payload#733

Merged
vbuberen merged 2 commits into
ChuckerTeam:developfrom
mreram:develop
Dec 8, 2021
Merged

Fix RTL issue in payload#733
vbuberen merged 2 commits into
ChuckerTeam:developfrom
mreram:develop

Conversation

@mreram

@mreram mreram commented Dec 4, 2021

Copy link
Copy Markdown
Contributor

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

vbuberen commented Dec 4, 2021

Copy link
Copy Markdown
Collaborator

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

@mreram

mreram commented Dec 4, 2021

Copy link
Copy Markdown
Contributor Author

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

Drjacky commented Dec 5, 2021

Copy link
Copy Markdown

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

MiSikora commented Dec 5, 2021

Copy link
Copy Markdown
Contributor

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

vbuberen commented Dec 5, 2021

Copy link
Copy Markdown
Collaborator

@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

Drjacky commented Dec 5, 2021

Copy link
Copy Markdown

@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

mreram commented Dec 5, 2021

Copy link
Copy Markdown
Contributor Author

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

mreram commented Dec 5, 2021

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

vbuberen commented Dec 8, 2021

Copy link
Copy Markdown
Collaborator

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.

5 participants