Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#26315 closed defect (bug) (fixed)

Grunt: RTL & CSSMin Color schemes

Reported by: dd32's profile dd32 Owned by: nacin's profile nacin
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: Cc:

Description (last modified by dd32)

Currently Colour schemes don't get RTL'd versions, which results in a very bland UI when using a non-default colour scheme in an RTL environment.

In addition, the colour schemes include @import url("../../colors-fresh.css"); which needs to dynamically change to -rtl.css so that minification includes the right file

Attachments (4)

26315.diff (1.4 KB) - added by dd32 11 years ago.
26315.2.diff (1.2 KB) - added by dd32 11 years ago.
26315.3.diff (357 bytes) - added by ocean90 11 years ago.
26315.4.diff (337 bytes) - added by ocean90 11 years ago.
props nacin

Download all attachments as: .zip

Change History (16)

#1 @dd32
11 years ago

  • Description modified (diff)

It looks like we'll need to add a processContent-like callback to the cssjanus grunt module to alter the include, or maybe a option to cssjanus to RTL'ise all include filenames it runs into.

I'm attaching my attempt at doing this, but it's not working. I'm not sure why, but color-schemes/*/colors-rtl.min.css appears to contain too much data, but colors-rtl.css and colors.min.css are fine.

Obviously, the -rtl.min includes the .min instead of the RTL version still since I couldn't figure out a way to process the content without altering something other than the grunt file.

I don't actually know what I'm doing with Grunt here.. so feel free to ignore the patch :)

@dd32
11 years ago

#2 @dd32
11 years ago

Created an upstream PR for grunt-cssjanus to add a processContent callback: https://github.com/yoavf/grunt-cssjanus/pull/2

26315.2.diff makes use of it and fixes my earlier issues with the generated files including too much content.

@dd32
11 years ago

#3 @dd32
11 years ago

Upon further thinking here.. It might be best to make all colour schemes not require a RTL variant at all.

This would mean removing cases like border-left: 4px solid #7ad03a; and replacing it with border-left-color: #7ad03a, and adding the appropriate border-left: declaration in the standard stylesheets, box-shadow: could cause a bit of pain though, as will gradients.

This has several benefits, the primary one being that 3rd party colour schemes will not have to include .rtl variants and we don't need to make significant changes to our existing colour stylesheet code.
In the event that we DO require 2 different colour borders on an element, we'd have to use .rtl ourselves in the stylesheet instead.

#4 @dd32
11 years ago

The more I look at not needing a -rtl variant of stylesheets and using .rtl instead, the more I realise one is needed, there's just too many styles to do it manually it seems.

3rd party colour schemes are going to have a seriously hard time trying to support RTL though, especially as they can't rely on @import like the core styles are using.

#5 @nacin
11 years ago

The upstream change to cssjanus has a typo and doesn't work.

Does this patch work otherwise? (Does need updating.)

#6 @dd32
11 years ago

The upstream change to cssjanus ​has a typo and doesn't work.

Correct, Looks like out of date code ended up in my commit branch. As long as it doesn't cause a JS error, that parameter is not needed for this ticket.

#7 @nacin
11 years ago

It does cause a JS error & Grunt fails.

#8 @nacin
11 years ago

Fixed with [26610].

#9 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 26616:

Build RTL stylesheets for color schemes.

props dd32.
fixes #26315.

#10 @ocean90
11 years ago

Running grunt cssjanus:colors produces files like this, if a -rtl.css styles exist already:

File "build/wp-admin/css/colors/blue/colors-rtl-rtl-rtl.css" created.
File "build/wp-admin/css/colors/blue/colors-rtl-rtl.css" created.
File "build/wp-admin/css/colors/blue/colors-rtl.css" created.
File "build/wp-admin/css/colors/light/colors-rtl-rtl-rtl.css" created.
File "build/wp-admin/css/colors/light/colors-rtl-rtl.css" created.
File "build/wp-admin/css/colors/light/colors-rtl.css" created.
File "build/wp-admin/css/colors/midnight/colors-rtl-rtl-rtl.css" created.
File "build/wp-admin/css/colors/midnight/colors-rtl-rtl.css" created.
File "build/wp-admin/css/colors/midnight/colors-rtl.css" created.

Doesn't happen when runnung cssjanus:core multiple times.

@ocean90
11 years ago

@ocean90
11 years ago

props nacin

#11 @nacin
11 years ago

As discussed in IRC, this occurs because we're doing build => build. cssjanus:core is src => build, thus it doesn't have this problem.

Simply restricting this to */colors.css is fine, then the existing colors-rtl.css file won't be caught.

#12 @nacin
11 years ago

In 26745:

Gruntfile: Prevent -rtl-rtl.css stylesheets when running cssjanus on colors. fixes #26315.

Note: See TracTickets for help on using tickets.