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: Permalink editor rtl languages #13919

Merged
merged 2 commits into from Sep 9, 2019

Conversation

@jorgefilipecosta
Copy link
Member

commented Feb 18, 2019

Description

Fixes: #12729
Improves the permalink editors on RTL languages.
The new design follows the screens shared by @desrosj in #12729.

How has this been tested?

I switched to an RTL language and verified the result I got is the one rendered on the screenshots.

Screenshots

Before:
screenshot 2019-02-18 at 12 56 54

After:
screenshot 2019-02-18 at 12 55 30

Before:
screenshot 2019-02-18 at 12 28 50

After:
screenshot 2019-02-18 at 12 07 12

@@ -9,3 +9,10 @@
.edit-post-post-link__link {
word-wrap: break-word;
}

/* rtl:begin:ignore */
body.rtl .edit-post-post-link__preview-link-container {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 18, 2019

Contributor

Ideally, we don't rely on body.rtl as these components can be shown anywhere and not only the editor pages. One way of doing this is to rely on the isRTL of the editor settings. But I'd like to understand why the default RTL handling was not working properly?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Feb 18, 2019

Author Member

Ideally, we don't rely on body.rtl as these components can be shown anywhere and not only the editor pages.
True I will update to use a class conditionally added by isRTL.

But I'd like to understand why the default RTL handling was not working properly?

The default handling was working properly the problem we have is that it looks like even on RTL languages the permalink is not RTL so we trying to force LTR on this zone.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 18, 2019

Contributor

Is there a way we could solve this just by using /* rtl:begin:ignore */ as I assume these styles could apply similarily to both directions right?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Feb 18, 2019

Author Member

Is there a way we could solve this just by using /* rtl:begin:ignore */ as I assume these styles could apply similarily to both directions right?

The problem is that direction rtl is inherited so the direction elements are rendered is changed and we need a rule to overwrite the direction in a specific zone.
I also verified that simply adding an ignore on the zone long-content-fade is used does not produces the ideal result. Probably that's the reason long-content-fade receives a direction parameter.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 18, 2019

Contributor

Can we force the direction here. Basically what I'm saying is: Can't we just keep these styles you added but remove body.rtl?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Feb 24, 2019

Author Member

Hi Riad, I updated this PR going in the direction you are suggesting, we now don't use body.rtl. Thank you for the feedback 👍

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Actually, this still appears broken to me when using RTL languages. I have this:

capture d ecran 2019-02-25 a 9 17 42 am

The URLS should show-up like the following screenshots (taken when using English in the dashboard)

capture d ecran 2019-02-25 a 9 18 12 am

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

Hi @youknowriad, on the RTL languages I'm getting this:
screenshot 2019-02-25 at 09 30 11
I think that is the expected result. The screenshots you provided look similar to what happens currently on master, maybe there was a caching issue?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

@jorgefilipecosta you're not using rtl characters in your example, maybe that's related, try some random arabic word in the title/permalink.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Also, why your Gutenberg UI don't seem translated?

@swissspidy

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Also, why your Gutenberg UI don't seem translated?

WordPress plugin language packs have been missing the needed JSON files in the past, but this should be fixed now. Try updating your translations.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

@jorgefilipecosta I wouldn't expect that result personally. I think the UI should still be reversed (from right to left) but the URL should not, the URL should always be shown from Left to Right.

@swissspidy

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Some feedback from actual RTL speakers might be beneficial here :-)

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Well, I am 🤷‍♂️ an RTL speaker

@swissspidy

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@youknowriad How could I forget, sorry! 🤦‍♂️

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

No worries 😄

@jorgefilipecosta jorgefilipecosta force-pushed the fix/permalink-editor-rtl branch 2 times, most recently from 3f9c8b9 to bccf734 Feb 27, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

Hi @youknowriad this PR was updated following your suggestion:
screenshot 2019-02-27 at 13 14 29
screenshot 2019-02-27 at 13 14 38

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

From the mockup, the white gradient seems like it's going in the wrong direction.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Hi @youknowriad, I changed the mockup direction:
image

But, to make the gradient work as expected I needed to change the text-align, otherwise, the gradient would always cover part of the link even if the link was small:
screenshot 2019-03-04 at 08 30 15

I'm not sure if this change is something acceptable.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/permalink-editor-rtl branch from cce2b46 to f0fd7dd Mar 22, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the fix/permalink-editor-rtl branch from f0fd7dd to f828147 Apr 12, 2019
@gziolo gziolo removed this from the 5.6 (Gutenberg) milestone May 6, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

@senadir is this something you can help review? It's a bit old but it seemed ready?

@jorgefilipecosta jorgefilipecosta force-pushed the fix/permalink-editor-rtl branch from f828147 to c1d0aa2 Aug 20, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

I just did a rebase the PR still seems to be valid and review would be helpful :)

@jorgefilipecosta jorgefilipecosta requested a review from senadir Sep 9, 2019
@senadir
senadir approved these changes Sep 9, 2019
Copy link
Contributor

left a comment

I took this in a long spin and it works great, ready to merge

@jorgefilipecosta jorgefilipecosta force-pushed the fix/permalink-editor-rtl branch from c1d0aa2 to 9c657d4 Sep 9, 2019
@jorgefilipecosta jorgefilipecosta merged commit 276981d into master Sep 9, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the fix/permalink-editor-rtl branch Sep 9, 2019
@youknowriad youknowriad added this to the Gutenberg 6.5 milestone Sep 14, 2019
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.