gjb2048 / moodle-theme_essential

The Essential Moodle Theme
https://gjb2048.github.io/moodle-theme_essential/
GNU General Public License v3.0
91 stars 120 forks source link

Implement RTL CSS switching. #249

Closed gjb2048 closed 10 years ago

gjb2048 commented 10 years ago

In the Shoehorn theme I have come up with a novel way of solving the LTR / RTL style sheet problem: https://moodle.org/mod/forum/discuss.php?d=264955 which I have also implemented in a feature branch of Bootstrap (V3): https://github.com/gjb2048/theme_bootstrap/tree/master_rtl

Therefore I intend to add this feature to Essential 2.7.8. The question is, should it be an option or a replacement?

gjb2048 commented 10 years ago

Notes:

Replacement.

Settings.less -> settings.css. Custom.less -> custom.css.

etc.

gjb2048 commented 10 years ago

Note to self & David: How to cope with editor.css RTL? - Humm - have to be all in one.

Also: moodle.less needs to be separated. So: Moodle.less -> moodle.css / moodle-rtl.css - new method with cssflip. Moodle-pix.less / Moodle-pix-rtl.less -> moodle-pix.css / moodle-pix-rtl.css - pre-process.

And Essential has some pix as well, so: Essential.less -> essential.css / essential-rtl.css - new method with cssflip. Contains FontAwesome. Settings.less -> settings.css - pre-process. Essential-pix.less / Essential-pix-rtl.less -> essential-pix.css / essential-pix-rtl.css - pre-process. Custom.less -> custom.css - pre-process. FontAwesome path.less -> fontawsome.css - pre-process. Alternatives are all settings, so keep as is and pre-process.

Remove all dir-rtl selectors unless determined to be needed and outside of the new method, i.e. specific RTL pix (moodle-pix-rtl.css). When removing, place in separate RTL.less -> rtl.css for the moment until mechanism proven. This is currently essential/rtl.less -> less/rtl.less - less/rtl/ moodle-rtl.less and essential-rtl.less.

Question: Should moodle.css and essential.css be combined into one such that one file served by new mechanism instead of two? I think so. Say theme.css and theme-rtl.css.

DBezemer commented 10 years ago

I'd call them essential.css and essential-rtl.css to make sure you have names that can be found easily I think the only way is to combine them as they both need the flip.

I think the only thing you haven't covered yet is what to do with existing RTL less in the moodle LESS files, otherwise it might cause the opposite effect

gjb2048 commented 10 years ago

Thanks. The plan with the existing RTL is to transfer it to another file and not serve, but keep as reference and put back bits as needed.=

gjb2048 commented 10 years ago

Note: custom.css is minimal, so no need for custom.less.

gjb2048 commented 10 years ago

Development now in: https://github.com/DBezemer/moodle-theme_essential/tree/master_249

I have placed the RTL CSS into a LESS structure referenced by: https://github.com/DBezemer/moodle-theme_essential/blob/master_249/less/moodle-rtl.less containing all of the RTL styles that I believe would not be flipped successfully and therefore needed to remain. They need to be whittled down by testing to see if they are still required in RTL and the flip technology has not worked for them.

Still need to make the layouts react Bootstrap grid wise. Apart from that, the mechanism has now been implemented and is flipping between essential.css and essential-rtl.css. Not tested with TDM off yet.

gjb2048 commented 10 years ago

Note to self:

Alternatives in Gruntfile.js Glyphicons essential.less form-action no import form.less Moodle RTL core.less / course.less not sure bit editor.css ??? odd. Layouts in RTL Rebase.

gjb2048 commented 10 years ago

Note: header.php has inclusion of essential.css before Moodle cached styles, this is so that their settings can override the defaults. However this means that the theme styles are no longer after the plugin styles and therefore some of those styles now override the theme styles. Therefore need to look out for this. A bit of a mess of contradictions with core plugin styles defining the same things that are in the bootstrap base styles. Better if there was a define once solution.

Or may need to move the styles that are not flipped into the theme's pre-processed collection. Humm. Going to leave as is for the moment as the set of styles in the plugin's is less than the theme itself.

gjb2048 commented 10 years ago

Ok, progress made. So far so good. Have sorted all 'note to self' above. Also updated header grid system with one of own such that logo flipping works. Additionally implemented cssflip 'at' replace syntax for changing values of font awesome icons for RTL rather than define a new selector - quite neat - see: https://github.com/DBezemer/moodle-theme_essential/compare/master_249#diff-0b9f7830217b2bbb5f4ae1f01576428dR158. I wonder if it could flip this: https://github.com/DBezemer/moodle-theme_essential/compare/master_249#diff-28dd365ba73d1c61e8b83b6d0292187eR7 too and save a selector or two.

gjb2048 commented 10 years ago

@nadavkav Please could you test this as posted on: https://moodle.org/mod/forum/discuss.php?d=269392

nadavkav commented 10 years ago

Downloaded. Installed. testing...

(Mostly, looks great! Though, I found some minor issues that needs to be fixed. I will report them over the weekend)

Great (great!) work :-)

On 11 September 2014 19:57, Gareth J Barnard notifications@github.com wrote:

@nadavkav https://github.com/nadavkav Please could you test this as posted on: https://moodle.org/mod/forum/discuss.php?d=269392

— Reply to this email directly or view it on GitHub https://github.com/DBezemer/moodle-theme_essential/issues/249#issuecomment-55294210 .

gjb2048 commented 10 years ago

Thanks Nadav, I knew there would be some glitches that are because of the new css order or simply existing issues. I have just updated the branch against latest problems but should not affect this. Thanks for testing.=

DBezemer commented 10 years ago

Awesome work @gjb2048 loads super fast and the total loaded CSS is a bit smaller than before

A few bugs I found: image blocks in RTL in footer

image overlap of header and dock

image Controls for docked blocks outside block region

image Breadcrumb reverse order (probably needs a lot of work to get right)

image Double margin on right, no margin on left of main region

image Section heads still LTR (probably Moodle core)

Overall awesome work tho, other than these minor things everything looks great :) Do you think there is any way to reduce the size of all combined CSS by removing exiting rtl styles?

nadavkav commented 10 years ago

I confirm all the above issues. should I suggest fixes, or are you already fixed them?

On 11 September 2014 23:47, David Bezemer notifications@github.com wrote:

Awesome work @gjb2048 https://github.com/gjb2048 loads super fast and the total loaded CSS is much much smaller than before

A few bugs I found: [image: image] https://cloud.githubusercontent.com/assets/1172670/4241391/5a84a6e4-39f3-11e4-9bc5-347b6a0d6a7e.png blocks in RTL in footer

[image: image] https://cloud.githubusercontent.com/assets/1172670/4241409/94616c6c-39f3-11e4-99d4-c85a4d24d8c0.png overlap of header and dock

[image: image] https://cloud.githubusercontent.com/assets/1172670/4241438/ddabbe54-39f3-11e4-9322-7a3c632f9d25.png Controls for docked blocks outside block region

[image: image] https://cloud.githubusercontent.com/assets/1172670/4241462/02672454-39f4-11e4-9659-78828d0f13b8.png Breadcrumb reverse order (probably needs a lot of work to get right)

[image: image] https://cloud.githubusercontent.com/assets/1172670/4241474/226f73a0-39f4-11e4-8ea1-b615a6b03fa1.png Double margin on right, no margin on left of main region

[image: image] https://cloud.githubusercontent.com/assets/1172670/4241513/8633b022-39f4-11e4-8ea3-122cf62a5836.png Section heads still LTR (probably Moodle core)

Overall awesome work tho, other than these minor things everything looks great :) Do you think there is any way to reduce the size of all combined CSS by removing exiting rtl styles?

— Reply to this email directly or view it on GitHub https://github.com/DBezemer/moodle-theme_essential/issues/249#issuecomment-55326256 .

gjb2048 commented 10 years ago

Thanks @DBezemer and @nadavkav - not fixed yet! It would be interesting to know if the Clean theme has the same issues and therefore understand the nature of how the fix should be implemented.

gjb2048 commented 10 years ago

Need to progressively remove RTL styles. Quite a bit has gone already with logic of how cssflip works.

gjb2048 commented 10 years ago

Note: preg_replace needed as in 'setting_file_url' in outputlib.php

gjb2048 commented 10 years ago

Fixed calendar issue. Updated serving code to be intelligent and only serve if file is needed when TDM off. Section heads is indeed a core issue as that comes from course format.

gjb2048 commented 10 years ago

Cannot replicate "overlap of header and dock" or "Controls for docked blocks outside block region" on Firefox or Chrome but can "Double margin on right, no margin on left of main region" only when using dock.

nadavkav commented 10 years ago

In clean, the dock is under the navbar, so it looks good and there is no need for the following code... fix for overlap of header and dock

.dir-rtl.has_dock .navbar {right: 42px;}
nadavkav commented 10 years ago

In theme Clean it looks fine. so, here is a fix:

image Fix:

.dir-rtl .block .calendar-controls .previous, 
.dir-rtl .block .calendar-controls .current, 
.dir-rtl .block .calendar-controls .next {float: left;}

image

.dir-rtl.path-calendar .calendar-controls .previous, 
.dir-rtl.path-calendar .calendar-controls .next, 
.dir-rtl.path-calendar .calendar-controls .current {float: left;}
gjb2048 commented 10 years ago

Thanks Nadav for the fixes, but the intent of this is to eliminate all 'dir-rtl' prefixing selectors and instead cssflip the LTR selectors such that they have the correct values when applied in RTL. So, the RTL version of the styles is a mirror image.

nadavkav commented 10 years ago

The breadcrumbs looks great on master branch. So I guess you should use that code.

BTW, I have disabled the animation on several servers because the teachers did not like it. You should have a mode for that on the listbox, I guess.

nadavkav commented 10 years ago

You are right. I forgot to trim it out from the example. feel free to remove the .dir-rtl. and just fix the code.

gjb2048 commented 10 years ago

This branch is following master and is rebased, so is just a matter of fixing stuff!

gjb2048 commented 10 years ago

With the animation, please raise a separate issue as an enhancement.

gjb2048 commented 10 years ago

Note: For calendar arrow issue, lib/outputlib.php - check_theme_arrows() does not appear to be doing the job of swapping!

Correction, it is but also appears the code that renders 'calendar-controls' class is also flipping, so double logic. Investigating.

Oh what a mess! It appears that /calendar/lib.php in calendar_top_controls flips left and right for RTL and also calls link_arrow_left() (also link_arrow_right()) in lib/weblib.php which in turn calls $OUTPUT->larrow() in /lib/outputlib.php who's constructor flips them! So a double flip! So that is a core issue that needs fixing! If one does not already exist.

nadavkav commented 10 years ago

Function check_theme_arrows() looks ok. And Moodle's theme Clean display it correctly. Are you sure?

On 13 September 2014 17:52, Gareth J Barnard notifications@github.com wrote:

Note: For calendar arrow issue, lib/outputlib.php - check_theme_arrows() does not appear to be doing the job of swapping!

— Reply to this email directly or view it on GitHub https://github.com/DBezemer/moodle-theme_essential/issues/249#issuecomment-55496113 .

gjb2048 commented 10 years ago

check_theme_arrows() is ok, but /calendar/lib.php on the frontpage also flips! And Essential larrow() is not overloaded in renderer.

gjb2048 commented 10 years ago

Ok, odd. Investigating.

Fair enough. Its Essential issue. Need a 'noflip'.

gjb2048 commented 10 years ago

Note: https://www.npmjs.org/package/flipcss package supports pseudo-element swapping. Currently working on blocks.less to do this manually. Need to investigate.

gjb2048 commented 10 years ago

Note: Fixed calendar locally, was Essential issue.

gjb2048 commented 10 years ago

Oh fudge! If lang actually rtl then before -> after automatically - well at least in display terms. Before is still before.

gjb2048 commented 10 years ago

Now fixed breadcrumb and one section per page issue. Now at stage where everything reported has been fixed. Branch rebased and updated. All please update your branches.

However... not sure how to fix this:

2014-09-13 18_41_17- _ ct c _ 2

Browser intelligently sorts 'before' pseudo-selector when using RTL text but when LTR text is used and yet direction is rtl then no swap applies.

nadavkav commented 10 years ago

Yee. that's a big question for me too. Technically, it is OK. but visually, it looks weird.

Practically, there should not be issues like this. Since English courses are set to an english language and the content is all English. and they are under English categories. (At least on most servers I manage). And Hebrew courses are the opposite. So it is safe to assume that only an Admin with a personal preference for English menus (UI) browsing an Hebrew system can have those UI mixes. And that is a minor issue, to my opinion. What do you think?

On 13 September 2014 20:47, Gareth J Barnard notifications@github.com wrote:

Now fixed breadcrumb and one section per page issue. Now at stage where everything reported has been fixed.

However... not sure how to fix this:

[image: 2014-09-13 18_4117- ct c _ 2] https://cloud.githubusercontent.com/assets/1058419/4261408/e0c61276-3b6d-11e4-9b5d-f57d0f07a596.png

Browser intelligently sorts 'before' pseudo-selector when using RTL text but when LTR text is used and yet direction is rtl then no swap applies.

— Reply to this email directly or view it on GitHub https://github.com/DBezemer/moodle-theme_essential/issues/249#issuecomment-55501271 .

nadavkav commented 10 years ago

One more "blocks flipping" issue... On a course setting page, switching languages from English to Hebrew, the blocks stay on the left side of the page. Which goes great with my "content first" UX concept. but should it flip sides? What do you think?

On 13 September 2014 21:02, Nadav Kavalerchik nadavkav@gmail.com wrote:

Yee. that's a big question for me too. Technically, it is OK. but visually, it looks weird.

Practically, there should not be issues like this. Since English courses are set to an english language and the content is all English. and they are under English categories. (At least on most servers I manage). And Hebrew courses are the opposite. So it is safe to assume that only an Admin with a personal preference for English menus (UI) browsing an Hebrew system can have those UI mixes. And that is a minor issue, to my opinion. What do you think?

On 13 September 2014 20:47, Gareth J Barnard notifications@github.com wrote:

Now fixed breadcrumb and one section per page issue. Now at stage where everything reported has been fixed.

However... not sure how to fix this:

[image: 2014-09-13 18_4117- ct c _ 2] https://cloud.githubusercontent.com/assets/1058419/4261408/e0c61276-3b6d-11e4-9b5d-f57d0f07a596.png

Browser intelligently sorts 'before' pseudo-selector when using RTL text but when LTR text is used and yet direction is rtl then no swap applies.

— Reply to this email directly or view it on GitHub https://github.com/DBezemer/moodle-theme_essential/issues/249#issuecomment-55501271 .

gjb2048 commented 10 years ago

Thanks Nadav.

With the course settings page the blocks should flip.

With the icon before issue I find it annoying, but in current absence of a solution then will have to live with it for the moment.=

gjb2048 commented 10 years ago

Just looking for possible solutions and perhaps: http://www.w3schools.com/cssref/pr_text_unicode-bidi.asp applied to the 'before' pseudo selector would work if set to 'bidi-override'

nadavkav commented 10 years ago

Do not think it is a good solution. See what happens: image

gjb2048 commented 10 years ago

Oh, not on the text. Please apply to the pseudo-selector only. Like the FontAwesome font property.

nadavkav commented 10 years ago

Me bad. You are right. works great :-)

gjb2048 commented 10 years ago

Thanks for testing Nadav, I've not had a chance to implement it yet.

Please could you raise the CSV issue separately. Then we can apply to the master branch and track changes.

gjb2048 commented 10 years ago

Humm, cannot get 'unicode-bidi: bidi-override;' to work on the before pseudo-selector.

gjb2048 commented 10 years ago

Note: Looked at overriding 'tree_block_contents' in 'theme_essential_core_renderer' which extends 'core_renderer' to have an alternative method of injecting the icon. But for this to happen the FontIcon needs to be within the text that is inside the tag of the tree item. This is generated by the call '$item->content($this)' which somehow accesses the content of the item which has been already created by 'get_content()' in the block. So cannot be done this way unless fiddle the string to find the first '>' and inject the code after that. Humm!

nadavkav commented 10 years ago

Sooo complicated :-( just leave it alone, for now. (Better invest your energies on other issues)

I must say, I am going through the theme, and it all looks very good!

On 14 September 2014 20:29, Gareth J Barnard notifications@github.com wrote:

Note: Looked at overriding 'tree_block_contents' in 'theme_essential_core_renderer' which extends 'core_renderer' to have an alternative method of injecting the icon. But for this to happen the FontIcon needs to be within the text that is inside the tag of the tree item. This is generated by the call '$item->content($this)' which somehow accesses the content of the item which has been already created by 'get_content()' in the block. So cannot be done this way unless fiddle the string to find the first '>' and inject the code after that. Humm!

— Reply to this email directly or view it on GitHub https://github.com/DBezemer/moodle-theme_essential/issues/249#issuecomment-55532635 .

gjb2048 commented 10 years ago

But is annoying! Tried this:

        $content = $item->content($this);
        $endofopentag = strpos($content, ">");
        error_log ($content.' - '.$endofopentag);
        $liclasses = array($item->get_css_type());
        if (!$item->forceopen || (!$item->forceopen && $item->collapse) || ($item->children->count()==0  && $item->nodetype==navigation_node::NODETYPE_BRANCH)) {
            $liclasses[] = 'collapsed';
            if ($endofopentag) {
                $content = substr($content, 0, $endofopentag).'<i class=\"fa fa-folder icon\" title=\"'.$item->get_title().'\">'.substr($content, $endofopentag);
            }
        }

And did not work.

gjb2048 commented 10 years ago

Ok, so if leave that for a moment, then everything is ok. Just needs testing more.

A hack to stop this would be to remove all of the 'before' stuff on the navigation tree. Ode to core providing a decent way for themes to override that icon on a per block basis. Oh hang on, could use the old fashioned background mechanism using extracted FontAwesome svg's. But that would mean fixed colour.

nadavkav commented 10 years ago

Fixed colour is not that bad according to Moodle "gray" design concept. But still, this is too unique an issue to spend your precious time on. leave it last on the list :-) (maybe Mary could help with the challenge?)

On 14 September 2014 20:53, Gareth J Barnard notifications@github.com wrote:

Ok, so if leave that for a moment, then everything is ok. Just needs testing more.

A hack to stop this would be to remove all of the 'before' stuff on the navigation tree. Ode to core providing a decent way for themes to override that icon on a per block basis. Oh hang on, could use the old fashioned background mechanism using extracted FontAwesome svg's. But that would mean fixed colour.

— Reply to this email directly or view it on GitHub https://github.com/DBezemer/moodle-theme_essential/issues/249#issuecomment-55533415 .

gjb2048 commented 10 years ago

@lazydaisy - any idea how to solve https://cloud.githubusercontent.com/assets/1058419/4261408/e0c61276-3b6d-11e4-9b5d-f57d0f07a596.png ?

lazydaisy commented 10 years ago

The CTC one you mean? Depends what the CSS is already. Guessing it is not styled for in RTL so it needs to be the opposite of what it is.