Details
Major
Description
In MDL-39824 a three column layout was introduced. When using the 'course category' course listing layout and only having blocks in 'side-pre' with an RTL language the layout is messed up. The block should be on the right and the content on the left.
Gliffy Diagrams
Attachments
Issue Links
Activity
- All
- Comments
- Work Log
- History
- Activity
- Links Hierarchy
I think I'm onto something as discovered that using a class 'region-bs-main-and-post' instead of 'region-bs-main-and-pre' works.
Ok, even though this works:
...
|
// Get the HTML for the settings bits.
|
$html = theme_clean_get_html_for_settings($OUTPUT, $PAGE);
|
|
|
if (right_to_left()) {
|
$regionbsid = 'region-bs-main-and-post';
|
} else {
|
$regionbsid = 'region-bs-main-and-pre';
|
}
|
|
|
echo $OUTPUT->doctype() ?>
|
<html <?php echo $OUTPUT->htmlattributes(); ?>>
|
<head>
|
<title><?php echo $OUTPUT->page_title(); ?></title>
|
<link rel="shortcut icon" href="<?php echo $OUTPUT->favicon(); ?>" />
|
<?php echo $OUTPUT->standard_head_html() ?>
|
<meta name="viewport" content="width=device-width, initial-scale=1.0">
|
</head>
|
|
|
<body <?php echo $OUTPUT->body_attributes(); ?>>
|
|
|
<?php echo $OUTPUT->standard_top_of_body_html() ?>
|
|
|
<header role="banner" class="navbar navbar-fixed-top<?php echo $html->navbarclass ?>">
|
<nav role="navigation" class="navbar-inner">
|
<div class="container-fluid">
|
<a class="brand" href="<?php echo $CFG->wwwroot;?>"><?php echo $SITE->shortname; ?></a>
|
<a class="btn btn-navbar" data-toggle="workaround-collapse" data-target=".nav-collapse">
|
<span class="icon-bar"></span>
|
<span class="icon-bar"></span>
|
<span class="icon-bar"></span>
|
</a>
|
<div class="nav-collapse collapse">
|
<?php echo $OUTPUT->custom_menu(); ?>
|
<ul class="nav pull-right">
|
<li><?php echo $OUTPUT->page_heading_menu(); ?></li>
|
<li class="navbar-text"><?php echo $OUTPUT->login_info() ?></li>
|
</ul>
|
</div>
|
</div>
|
</nav>
|
</header>
|
|
|
<div id="page" class="container-fluid">
|
|
|
<header id="page-header" class="clearfix">
|
<div id="page-navbar">
|
<nav class="breadcrumb-button"><?php echo $OUTPUT->page_heading_button(); ?></nav>
|
<?php echo $OUTPUT->navbar(); ?>
|
</div>
|
<?php echo $html->heading; ?>
|
<div id="course-header">
|
<?php echo $OUTPUT->course_header(); ?>
|
</div>
|
</header>
|
|
|
<div id="page-content" class="row-fluid">
|
<div id="<?php echo $regionbsid ?>" class="span9">
|
<div class="row-fluid">
|
<section id="region-main" class="span8 pull-right">
|
<?php
|
echo $OUTPUT->course_content_header();
|
echo $OUTPUT->main_content();
|
echo $OUTPUT->course_content_footer();
|
?>
|
</section>
|
<?php echo $OUTPUT->blocks('side-pre', 'span4 desktop-first-column'); ?>
|
</div>
|
</div>
|
<?php echo $OUTPUT->blocks('side-post', 'span3'); ?>
|
</div>
|
....
|
With LTR the content fills the whole page and with RTL you are left with a blank area on the left. With LTR even though the content occupies all of the page it is still possible to drag blocks to the right as the target still exists. So need some sort of mirror job because the critical bit is that the block that is shown must be within the inner 'row-fluid' for this to work.
Ok, bingo, just needed to adapt the style in 'core.less' but as that's in the area of MDL-40065 will need to hold off to avoid putting back what I've taken out. In the mean time will publish the solution here in the 'master' branch for comment.
Hold your horses Gareth. You cannot make these changes without thinking about this some more.
1. You cannot make the width of span8 100% as its width is pre-determined by nature of its name: span8 + span4 = span12
2. If span8 is all alone then span9 takes over and combined with span3 is either span3 + span9 (side-pre-only) or span9 + span3 (side-post-only) which is same as column2.php logic for RTL.
Which is the way I did this in Tiny, but was told I was wrong! 
https://github.com/lazydaisy/Moodle-Studio/blob/tiny/tiny/style/tiny-layout.css
Dear Mary Evans,
RE:
1. You cannot make the width of span8 100% as its width is pre-determined by nature of its name: span8 + span4 = span12
2. If span8 is all alone then span9 takes over and combined with span3 is either span3 + span9 (side-pre-only) or span9 + span3 (side-post-only) which is same as column2.php logic for RTL.
True'ish, but it has precedence with the LTR code:
.empty-region-side-post #region-bs-main-and-pre.span9 {
|
width:100%;
|
}
|
which is still there despite:
.empty-region-side-post.used-region-side-pre #region-main.span8 {
|
/** increase the span size by 1 **/
|
.fluid-span(9);
|
}
|
.empty-region-side-post.used-region-side-pre #block-region-side-pre.span4 {
|
/** decrease the span size by 1 **/
|
.fluid-span(3);
|
}
|
But I found that in RTL with no side post the code:
.empty-region-side-post.used-region-side-pre #region-main.span8 {
|
/** increase the span size by 1 **/
|
.fluid-span(9);
|
}
|
Only gave a content width of 74.ish% and therefore there was white space on the left, hence:
.empty-region-side-post #region-bs-main-and-pre.span9 {
|
width:100%;
|
}
|
And I cannot implement 'column2.php' logic as that leads to no 'display:none;' block available as a drop target when editing. So I have to stick with the nested 8+4 in the 9+3.
I do see how you have done this in a different way
Personally, I think all of this (not your code in particular - the css grid fiddle way) the wrong way of going about it. I think we should keep the CSS as true to Bootstrap as possible, not add grid changes and do our adaptions purely in the layout files. In this way it will be easier to maintain and be more obvious to the theme designer what is going on.
Cheers,
Gareth
I believe you are right for once. It is far too complicated. It would actually be better all round if Martin stopped wanting Bootstrap to be a new base theme, because it is NOTHING like Base theme. In fact it would work better as a responsive theme if it were a side-post-only theme. then the region-main would be seen first and the blocks last. The only page that would need to be different is the report page as the Admin pages look better as side-post-only as I stated in MDL-40065. So we could do away with the three column theme which is not really Bootstrap. If anything it should have a a better looking Frontpage that is 'customisable', and at least three containers just above the footer, and again 'customisable'. In other words like this... http://twitter.github.io/bootstrap/examples/hero.html
I believe you are right for once. It is far too complicated. It would actually be better all round if Martin stopped wanting Bootstrap to be a new base theme, because it is NOTHING like Base theme. In fact it would work better as a responsive theme if it were a side-post-only theme. then the region-main would be seen first and the blocks last. The only page that would need to be different is the report page as the Admin pages look better as side-post-only as I stated in MDL-40065. So we could do away with the three column theme which is not really Bootstrap. If anything it should have a a better looking Frontpage that is 'customisable', and at least three containers just above the footer, and again 'customisable'. In other words like this... http://twitter.github.io/bootstrap/examples/hero.html
Thanks for working on this, guys.
I think I have fixed it...but not sure. Will post what I have done and see what you think.
Dear Mary Evans,
Thank you, some interesting food for thought - I'm intrigued as to the fix given the link to the 'hero' template.
I disagree that Bootstrap should not be a base theme but rather I think that there should be less code in the 'less/moodle' folder - no pun intended 
. It certainly has driven more child themes and given theme designers confidence to create them rather on being uncertain about a contributed parent.
I think the real issue here is templating in layouts and having the power to adapt to different situations rather than attempting a one size fits all solution.
And I'm right quite a lot of the time, it's just not often recognised
Cheers,
Gareth
Gareth J Barnard, I moved my comment from MDL-40065, here. So here goes:
Please check that when DND blocks from left (side-pre) column to right (side-post) column, position is saved after turning editing mode. (I remember we had issues with that in the past. lib/ajax/blocks.php , action=="move")
Dear Nadav Kavalerchik,
Just tested the turn editing off scenario and seems to be fine with this fix.
Cheers,
Gareth
OK...here is what I was doing last night.
|
|
<div id="page-content" class="row-fluid">
|
<div id="region-main" class="span9
|
<?php
|
if ('right_to_left'){ ?>
|
pull-left <?php
|
} else { ?>
|
pull-right
|
<?php
|
} ?>">
|
<div class="row-fluid">
|
<section id="region-main" class="span8
|
<?php
|
if ('right_to_left'){ ?>
|
pull-right <?php
|
} else { ?>
|
pull-left
|
<?php
|
} ?>">
|
<?php
|
echo $OUTPUT->course_content_header();
|
echo $OUTPUT->main_content();
|
echo $OUTPUT->course_content_footer();
|
?>
|
</section>
|
<?php echo $OUTPUT->blocks('side-pre', 'span4 desktop-first-column'); ?>
|
</div>
|
</div>
|
<?php echo $OUTPUT->blocks('side-post', 'span3');?>
|
</div>
|
|
|
|
In effect this just changes the way the page is is displayed.
The changes to the .less file is more complex. I'm not sure I have the right however I have added the un-compiled moodle.css if someone wants to test this out.
Dear Mary Evans,
Thanks - off to lunch, back soon .
Cheers,
Gareth
P.S. Code needs to toggle on / off 'desktop-first-column' too
P.P.S. I'll see what 'moodle.css' does.
Dear Mary Evans,
Looking at your changed 'moodle.css', the selector:
.empty-region-side-post #region-main {
|
width: 100%;
|
}
|
breaks the layout on the two column layout 'column2.php' file because it is not specific enough to the '#region-bs-main-and-post' identifier which is used in 'columns3.php' and '#region-bs-main-and-post' will not be needed in two column layouts as I believe it's only needed in three column layouts.
On the three column layout in LTR it appears to be overridden by '.empty-region-side-post.used-region-side-pre #region-main.span8' which is set at '74%' and so the content does not fill the page when there is no post side. Ditto for RTL.
Cheers,
Gareth
Yes...that's when I called it a day when I heard the dawn chorus, and saw that the page was not displaying correctly.
Also...I just read that MDL-40065 has been reopened...
Oh that is a blow!
Ah well...that's what make it fun.
I'm not really sure that Sam has it correct either.
I think we are nearly there with the 3col layout. Just needs tweaking. Like the desktop class for rtl which I have missed, and the css.
Dear Mary Evans,
Indeed - you must have been operating on high caffeine to stay up that long!
I think between us we nearly have this licked - just really seeing the outcome of MDL-40065 to know completely what to do with this. I'm not totally convinced either as you can see from my comments - but have left the old branches available in case they are the solution as against the hybrid with Sam's non-swapping detection code. More of a compromise. But both fixes work.
With the 'desktop class' issue, if code has more than one place that depends on the outcome of a decision I tend to put the outcome of the decision in a variable and then use it - hence the way my code works. Then if the decision takes a lot of calculation then there is not a performance hit. I know this one probably does not, but if it did in future then the code would still be efficient.
Can you re-peer review MDL-40065 to get it through to integration review please?
Cheers,
Gareth
I was just about to fix a less/moodle/... file on some other RTL issue I opened. And one of my fellow developers was looking over at the fix and asked me if we are working on an RTL Bootstrap code. in the first place.
So, Just wondering out load...
Did you use a bootstrap that was already RTL "enabled" like one of the following : bootstrap rtl
Are we trying to fix (bend) an LTR Bootstrap with no RTL framework?
Dear Nadav Kavalerchik,
Umm, to be honest I'm not sure what you are talking about with '(bend) an LTR Bootstrap with no RTL framework' - all I'm doing is fixing a problem with Bootstrap at the Moodle end of the framework in terms of ensuring the layout files for Bootstrapbase work with both LTR and RTL. So the fixes are at the Moodle end. I've not gone near the Bootstrap end bar to use the existing classes and make adaptions at the Moodle end.
Cheers,
Gareth
I was about to change some padding-right to padding-left in one of moodle's less files, And a fellow theme developer in my team saw me and told me that is not the right way to make a bootstrap RTL fix. And that we need to use a proper RTL enabled bootstrap in the first place. So I am forwarding the question here. Does the original bootstrap code that we are using here has RTL/LTR capabilities in the first place?
Dear Nadav Kavalerchik,
Umm, I still don't know the answer to that one (as per my previous comment) - please ask David Scotson and Bas Brands - but this issue is not related to the imported Bootstrap code as such but the structure of the Moodle layout files that support it. My understanding of the Moodle 'less' files is that there is RTL bits in there and I think there is 'RTL' styles in the Bootstrap 'less' files that Moodle uses. Have a look - either way it will not affect this because this fixes a Moodle layout file issue in using Bootstrap.
Cheers,
Gareth
Ok. Thank
I will take this question to David and Bas.
Hi Gareth,
It'd be really great to avoid the need to add php code back into these layout files.
I'd much prefer to solve the problem in CSS if possible and it looks to me like it would be.
Would something like the following work instead of changing #region-bs-main-and-pre => #region-bs-main-and-post?
.dir-rtl.three-column.empty-region-side-post #region-bs-main-and-pre.span9 #region-main.span8 {
|
/** RTL with no post area. **/
|
width:100%;
|
}
|
Also just noting that if we're making changes to the layout files in clean we've probably also got to make them to the bootstrapbase layout files.
I'll leave this in integration review so that we can discuss it.
Many thanks
Sam
Dear Sam Hemelryk,
True, but:
.dir-rtl.three-column.empty-region-side-post #region-bs-main-and-pre.span9 #region-main.span8 {
|
/** RTL with no post area. **/
|
width:100%;
|
}
|
Is not the full story, you need to apply '#region-bs-main-and-post' to the layout instead of '#region-bs-main-and-pre' in RTL as it pulls the areas into the right position. I was pulling my hair out for ages over this before I found the '#region-bs-main-and-post' selector. Please just try yours and you'll see what I mean!
Also, I do intend a Bootstrapbase layout file and M2.5 branch, just put this up for Peer review to get opinion before duplicating lots of work.
Cheers,
Gareth
I'm now going to see how MDL-40065 goes and what decisions are made there before proceeding here.
Hi Gareth,
I've been looking at this one again this morning.
The CSS I purposed above doesn't work, having spent more time actually looking at what is going on I'm sure it is possible to solve the problem in CSS. I suspect tidying up the CSS I wrote originally and fixing the problem would be possible path.
However I'm very much on the fence as to which way is best to proceed.
It appears to me either we'll need a complex CSS solution, or we can add a class to allow us to avoid CSS for and-pre and instead implement specific CSS for and-post.
The complex CSS leads to a "cleaner" layout file but really hides all of the magic and will no doubt require more time in understanding and care in maintaining.
The class swap leads to PHP in the layout file (which we've tried to limit) but puts reduces the "magic" required in CSS, will be an easier solution to understand and maintain.
At this point I am happy to take either solution, so if you'd like me to integrate your current solution here just let me know so, otherwise ping me when you've got a CSS solution in place.
Either way this issue definitely needs a resolution before 2.5.1 which is coming up shortly.
Many thanks
Sam
Dear Sam Hemelryk,
I completely agree with what you are saying (including on the minimal PHP front). And indeed although a pure CSS solution is the best technically but there is the human element to consider as I can just imagine a whole load of questions on the theme's forum to answer and not in a few sentences - lots of work for Mary et al. Therefore I'm happy with the solution (until somebody else comes up with a better idea) as I have it as it fulfills the two requirements of fixing the issue and being easy to understand.
I think with the CSS we use with Bootstrap we should keep the alterations on top of it to a minimum to facilitate a perceived reduction in maintenance issues when upgrading and supporting core changes. I think renderers will help with this.
I don't think Moodle should be about smoke and mirrors rather transparency. The Moodle magic should be in plain sight for all to see.
Do you need a MOODLE_25_STABLE branch or can you just use the supplied branch for both? If the former, then please give me 10-11 hours as just gone to bed!
Thanks,
Gareth
Thanks Gareth, I've applied these changes to both MOODLE_25_STABLE and master.
I've also applied your changes in theme/clean/layout/columns3.php to theme/bootstrapbase/layout/columns3.php.
Many thanks
Sam
Nice and neat...well done. I am glad that we are making some progress with these new layouts.
By the way...was it intentional or an oversight that the logo and footnote got missed off the new layout files for clean theme?
Can someone create a new tracker then I can fix them ready for next weeks pull.
Cheers
Ah - thanks for noting that Mary.
As requested I've created MDL-40316 now and will assign it to you shortly.
I'll keep an eye out for it next week
Many thanks
Sam
Dear Mary Evans and Sam Hemelryk,
The regression that caused MDL-40316 was purely an oversight on my part. I had initially put the new changed file in the wrong folder in the first place. My mistake.
Thanks Sam for taking this patch much further with all the work on MDL-40065 I'd forgotten to provide the full implementation.
Cheers,
Gareth
Tested and passed thanks Gareth.
Thanks for giving me joys and smiles
Thanks for sharing my trouble's pileThanks for wipeing the tears of my eye
Thanks for showing me the glad view of skyThanks for lending me your shoulders to lean
Thanks for giving my words a proper meanThanks for telling me the value of life
Thanks for showing me the rules to surviveThanks for lending me the sympathetic ears
Thanks for showing how much you careFrom all this what I mean in the end
Is thanks for being my special friend.– Seema Chowdhury
Sent upstream so... closing, thanks!
Dear Mary Evans and Sam Hemelryk,
Discovered whilst looking at
MDL-39824with Shoelace. Perhaps a solution similar toMDL-40065second fix or along the lines of it's first fix -> https://github.com/gjb2048/moodle/compare/master...wip-MDL-40065_masterCheers,
Gareth