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

#12170 Set up RTL generation and enqueuing #12917

Merged
merged 9 commits into from
Feb 8, 2017
Merged

#12170 Set up RTL generation and enqueuing #12917

merged 9 commits into from
Feb 8, 2017

Conversation

claudiulodro
Copy link
Contributor

This relates to issue #12170.

I've set up the Gruntfile for generating RTL CSS files and modified the style enqueuing to handle RTL files when appropriate. The auto-generated RTL files seem to work great out of the box. The only tweak I see that will need to be done is that the Reviews tab on products shows up as "(Reviews (4" in RTL mode.

To verify, I recommend https://wordpress.org/plugins/rtl-tester/

@claudiosanches
Copy link
Contributor

@jameskoster do you want check this one?

@mikejolley
Copy link
Member

For some reason in Chrome the admin products screen is blank until I inspect. Odd, needs @jameskoster to review.

@mikejolley mikejolley mentioned this pull request Jan 25, 2017
4 tasks
Copy link
Member

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

All looks good on the frontend, only thing I noticed was this;

There are a few issues on the backend;

I also noticed there are issues with pretty much all the select2 elements (multi-select, general dropdowns etc). This affects both front and backend.

@mikejolley I occasionally see a similar issue - a blank screen, until I scroll (or open inspector). I'm wondering if that's cause by the rtl-tester. You didn't experience that @claudiulodro ? I found this happened on most of the admin screens (including non-wc screens) and a couple of the frontend pages (orders in my account, for one).

@claudiulodro
Copy link
Contributor Author

@jameskoster, @mikejolley I haven't been able to reproduce the blank screen thing. I don't see a way it could be coming from the RTL Tester plugin, though, since that only has about 10 lines of code that does anything, and it's all PHP. Most likely it's something with Chrome. Do you still get this problem in another browser?

@jameskoster what is your recommended solution for the visual tweaks that have to be done on top of the auto-generated RTL CSS? Modifying the generated RTL with properties like Storefront does? https://github.com/woocommerce/storefront/blob/master/Gruntfile.js

@jameskoster
Copy link
Member

I reckon most of the issues can be fixed in the ltr stylesheet. Seems like they're caused by things like alignment and margins.

For anything that can't be fixed there, yes that's the approach we should use I think.

I don't see the blank screen issue in Firefox so yeah, probably a Chrome thing.

@mikejolley mikejolley added status: in progress This is being worked on. and removed [!] Needs Review / Feedback labels Feb 6, 2017
@mikejolley
Copy link
Member

I've changed the label to in progress. Mark it for review again when ready :)

@claudiulodro claudiulodro added [!] Needs Review / Feedback and removed status: in progress This is being worked on. labels Feb 6, 2017
@claudiulodro
Copy link
Contributor Author

@jameskoster I've changed the CSS to handle the areas you noticed need work. I wasn't able to reproduce the wc notices styling error you reported (Works great in RTL mode for me on all browsers). I also had to do some tweaks to get the tiptips working properly in RTL, so that's included in the changes.

@mikejolley
Copy link
Member

This looks good to me now, and code is fine. LGTM 👍

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.

4 participants