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

Improve RTL rendering #3831

Closed
wants to merge 5 commits into from
Closed

Improve RTL rendering #3831

wants to merge 5 commits into from

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Dec 6, 2017

Description

This PR improves the RTL appearance of Gutenberg:

screen shot 2017-12-06 at 13 09 05

Additionally, it might fix #1565, as it also flips horizontally the arrow icon.

How Has This Been Tested?

Chrome on Mac.

Steps to test

  • Set your admin language to an RTL language, like Arabic
  • Edit or write a post in Gutenberg

@jasmussen jasmussen added the General Interface Parts of the UI which don't fall neatly under other labels. label Dec 6, 2017
@jasmussen jasmussen self-assigned this Dec 6, 2017
Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with the filter property, seems it looks ok.

However, I'm getting a PHP warning when I switch to RTL.

<b>Warning</b>:  strpos() expects parameter 1 to be string, array given in
<b>/srv/www/wordpress-develop/public_html/build/wp-includes/functions.php</b> on line <b>3451</b><br />

Seems when Gutenberg is activated, $mce_init['plugins'] in the core _mce_set_direction() is no longer a string but an array. When Gutenberg is deactivated, it's a string, as expected.

@youknowriad
Copy link
Contributor

I haven't tested yet, but by looking at the screenshot, I think we should switch the "undo/redo" icons in RTL

@jasmussen
Copy link
Contributor Author

Great feedback, both — I'll look at how to best flip around undo/redo, though I think I'll need help fixing that PHP error. I assumed it was an issue with my local docker.

@youknowriad
Copy link
Contributor

I pushed a fix for the php error.

About the styling in RTL. I saw a lot of places where we need to change padding/marging from left to right etc...

screen shot 2017-12-07 at 09 25 31

screen shot 2017-12-07 at 09 25 23

screen shot 2017-12-07 at 09 25 07

screen shot 2017-12-07 at 09 25 02

screen shot 2017-12-07 at 09 24 53

I'm probably missing some other places as well, I wonder if it's not better to just use something like https://www.npmjs.com/package/postcss-rtl

@jasmussen
Copy link
Contributor Author

Thanks so much Riad.

I'm probably missing some other places as well, I wonder if it's not better to just use something like https://www.npmjs.com/package/postcss-rtl

I think we absolutely should! Do you have time to help me out with this? It's not super urgent.

@youknowriad
Copy link
Contributor

@jasmussen Sure let me try it :)

@jasmussen
Copy link
Contributor Author

🏅

@youknowriad
Copy link
Contributor

Update with the postcss plugin, it looks like it works properly. We still have some issues (popovers) because these are JS generated styles but most of the issues are solved.

What do you think?

@jasmussen
Copy link
Contributor Author

I think this is incredibly impressive, and thanks so much for doing this. I think it's a solid update, and brings us to a point where we can have individual highly specific tickets for each remaining RTL item, instead of one big mammoth umbrella "fix RTL" ticket. 👍 👍 and thanks again

@youknowriad
Copy link
Contributor

@jasmussen It looks like I broke non-rtl languages :) looking

@youknowriad
Copy link
Contributor

youknowriad commented Dec 7, 2017

WP adds a dir=rtl to the html tag when using an rtl language, the postcss plugin rely on it but also on a dir=ltr on ltr languages. I added a small "hack" to fix this but this should be moved to WP Core on merge.

There's also a small priority issue I had to fix for buttons.

@youknowriad youknowriad requested a review from aduth December 7, 2017 11:05
@yoavf yoavf mentioned this pull request Dec 7, 2017
3 tasks
@youknowriad
Copy link
Contributor

#3844 is merged, let's close this one. Good job everyone

@youknowriad youknowriad closed this Dec 7, 2017
@youknowriad youknowriad deleted the try/rtl-improvements branch December 7, 2017 15:39
@jasmussen
Copy link
Contributor Author

🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Meta boxes arrow icons in RTL
3 participants