Moodle

Bootstrap three column layout does not render correctly in RTL when only one block.

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 2.5
  • Fix Version/s: 2.5.1
  • Component/s: Themes
  • Labels:

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.

  1. File
    moodle.css
    17/Jun/13 12:19 PM
    336 kB
    Mary Evans
  1. mdl-40204.png
    39 kB
    16/Jun/13 3:52 PM

Issue Links

discovered while testing

Improvement - An enhancement to an existing Moodle feature. MDL-39824 Simplification of themes

  • Critical - Crashes server, loss of data, severe memory leak
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
is blocked by

Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-40065 Bootstrap Theme always sends content to "side-pre", even when there isn't one

  • Blocker - Blocks development and/or testing, prevents Moodle from running (applicable to bugs only)
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Gareth J Barnard added a comment -

Dear Mary Evans and Sam Hemelryk,

Discovered whilst looking at MDL-39824 with Shoelace. Perhaps a solution similar to MDL-40065 second fix or along the lines of it's first fix -> https://github.com/gjb2048/moodle/compare/master...wip-MDL-40065_master

Cheers,

Gareth

Show
Gareth J Barnard added a comment - Dear Mary Evans and Sam Hemelryk , Discovered whilst looking at MDL-39824 with Shoelace. Perhaps a solution similar to MDL-40065 second fix or along the lines of it's first fix -> https://github.com/gjb2048/moodle/compare/master...wip-MDL-40065_master Cheers, Gareth
Hide
Permalink
Gareth J Barnard added a comment -

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.

Show
Gareth J Barnard added a comment - 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.
Hide
Permalink
Gareth J Barnard added a comment - - edited

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.

Show
Gareth J Barnard added a comment - - edited 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.
Hide
Permalink
Gareth J Barnard added a comment -

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.

Show
Gareth J Barnard added a comment - 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.
Hide
Permalink
Mary Evans added a comment - - edited

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.

Show
Mary Evans added a comment - - edited 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.
Hide
Permalink
Mary Evans added a comment -

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

Show
Mary Evans added a comment - 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
Hide
Permalink
Gareth J Barnard added a comment - - edited

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

Show
Gareth J Barnard added a comment - - edited 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
Hide
Permalink
Mary Evans added a comment -

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

Show
Mary Evans added a comment - 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
Hide
Permalink
Mary Evans added a comment -

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

Show
Mary Evans added a comment - 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
Hide
Permalink
Michael de Raadt added a comment -

Thanks for working on this, guys.

Show
Michael de Raadt added a comment - Thanks for working on this, guys.
Hide
Permalink
Mary Evans added a comment -

I think I have fixed it...but not sure. Will post what I have done and see what you think.

Show
Mary Evans added a comment - I think I have fixed it...but not sure. Will post what I have done and see what you think.
Hide
Permalink
Gareth J Barnard added a comment - - edited

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

Show
Gareth J Barnard added a comment - - edited 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
Hide
Permalink
Nadav Kavalerchik added a comment -

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")

Show
Nadav Kavalerchik added a comment - 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")
Hide
Permalink
Gareth J Barnard added a comment -

Dear Nadav Kavalerchik,

Thanks - will do.

Gareth

Show
Gareth J Barnard added a comment - Dear Nadav Kavalerchik , Thanks - will do. Gareth
Hide
Permalink
Gareth J Barnard added a comment -

Dear Nadav Kavalerchik,

Just tested the turn editing off scenario and seems to be fine with this fix.

Cheers,

Gareth

Show
Gareth J Barnard added a comment - Dear Nadav Kavalerchik , Just tested the turn editing off scenario and seems to be fine with this fix. Cheers, Gareth
Hide
Permalink
Mary Evans added a comment - - edited

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.

Show
Mary Evans added a comment - - edited 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.
Hide
Permalink
Gareth J Barnard added a comment - - edited

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.

Show
Gareth J Barnard added a comment - - edited 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.
Hide
Permalink
Gareth J Barnard added a comment -

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

Show
Gareth J Barnard added a comment - 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
Hide
Permalink
Mary Evans added a comment -

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.

Show
Mary Evans added a comment - 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.
Hide
Permalink
Gareth J Barnard added a comment -

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

Show
Gareth J Barnard added a comment - 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
Hide
Permalink
Nadav Kavalerchik added a comment -

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?

Show
Nadav Kavalerchik added a comment - 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?
Hide
Permalink
Gareth J Barnard added a comment -

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

Show
Gareth J Barnard added a comment - 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
Hide
Permalink
Nadav Kavalerchik added a comment -

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?

Show
Nadav Kavalerchik added a comment - 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?
Hide
Permalink
Gareth J Barnard added a comment -

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

Show
Gareth J Barnard added a comment - 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
Hide
Permalink
Nadav Kavalerchik added a comment -

Ok. Thank
I will take this question to David and Bas.

Show
Nadav Kavalerchik added a comment - Ok. Thank I will take this question to David and Bas.
Hide
Permalink
Sam Hemelryk added a comment -

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

Show
Sam Hemelryk added a comment - 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
Hide
Permalink
Gareth J Barnard added a comment -

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

Show
Gareth J Barnard added a comment - 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
Hide
Permalink
Gareth J Barnard added a comment -

I'm now going to see how MDL-40065 goes and what decisions are made there before proceeding here.

Show
Gareth J Barnard added a comment - I'm now going to see how MDL-40065 goes and what decisions are made there before proceeding here.
Hide
Permalink
Sam Hemelryk added a comment -

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

Show
Sam Hemelryk added a comment - 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
Hide
Permalink
Gareth J Barnard added a comment - - edited

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

Show
Gareth J Barnard added a comment - - edited 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
Hide
Permalink
Sam Hemelryk added a comment -

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

Show
Sam Hemelryk added a comment - 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
Hide
Permalink
Mary Evans added a comment -

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

Show
Mary Evans added a comment - 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
Hide
Permalink
Sam Hemelryk added a comment -

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

Show
Sam Hemelryk added a comment - 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
Hide
Permalink
Gareth J Barnard added a comment -

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

Show
Gareth J Barnard added a comment - 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
Hide
Permalink
Sam Hemelryk added a comment -

Tested and passed thanks Gareth.

Show
Sam Hemelryk added a comment - Tested and passed thanks Gareth.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Thanks for giving me joys and smiles
Thanks for sharing my trouble's pile

Thanks for wipeing the tears of my eye
Thanks for showing me the glad view of sky

Thanks for lending me your shoulders to lean
Thanks for giving my words a proper mean

Thanks for telling me the value of life
Thanks for showing me the rules to survive

Thanks for lending me the sympathetic ears
Thanks for showing how much you care

From all this what I mean in the end
Is thanks for being my special friend.

– Seema Chowdhury

Sent upstream so... closing, thanks!

Show
Eloy Lafuente (stronk7) added a comment - Thanks for giving me joys and smiles Thanks for sharing my trouble's pile Thanks for wipeing the tears of my eye Thanks for showing me the glad view of sky Thanks for lending me your shoulders to lean Thanks for giving my words a proper mean Thanks for telling me the value of life Thanks for showing me the rules to survive Thanks for lending me the sympathetic ears Thanks for showing how much you care From all this what I mean in the end Is thanks for being my special friend. – Seema Chowdhury Sent upstream so... closing, thanks!

People

Vote (0)
Watch (6)

Dates

  • Created:
    Updated:
    Resolved: