Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37645

Generalise the CSS for moodle dialogues

    Details

    • Testing Instructions:
      Hide
      • Open a course
      • Turn editing on
      • Open the activity chooser
        • Confirm that it all looks fine
      • edit /enrol/ajax.php
        • After the define('AJAX_SCRIPT', true); line, add:

          throw new moodle_exception('mdl-37645');
          

      • open Course administration -> Users -> Enrolled users
        • Confirm that the styling on the exception is pleasing to the eye and congruent with other Moodle dialogues
      Show
      Open a course Turn editing on Open the activity chooser Confirm that it all looks fine edit /enrol/ajax.php After the define('AJAX_SCRIPT', true); line, add: throw new moodle_exception('mdl-37645'); open Course administration -> Users -> Enrolled users Confirm that the styling on the exception is pleasing to the eye and congruent with other Moodle dialogues
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      Recent JS dialogue work in Moodle has started to make heavier use of M.core.dialogue provided by lib/yui/notification/notification.js.

      At present, use of this dialogue/class is largely limited to:

      • chooser dialogues (activity chooser); and
      • JS exceptions/errors.

      I'm also working on MDL-35819 which also looks to use the same dialogue for all help popups in Moodle.

      I've done some work on the theming of these dialogues to make them more consistent with the rest of Moodle, so submitting to the wrath of the theme team.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment -

            This is in preparation for MDL-35819 which will use these settings, and add one or two more which aren't currently in use.

            Show
            dobedobedoh Andrew Nicols added a comment - This is in preparation for MDL-35819 which will use these settings, and add one or two more which aren't currently in use.
            Hide
            lazydaisy Mary Evans added a comment -

            Why wrath? Do you feel like the Sword of Damocles is over you or something? LOL

            Show
            lazydaisy Mary Evans added a comment - Why wrath? Do you feel like the Sword of Damocles is over you or something? LOL
            Hide
            lazydaisy Mary Evans added a comment -

            Actually Andrew it looks OK but cant test it right now as about to have my tea.

            One thing that jumped out at me is the margin in this line:

            .moodle-dialogue-base .moodle-dialogue-wrap .moodle-dialogue-ft {
                margin: 0.7em 1em;
                text-align: right;
                background-color: #FFF;
                font-size: 12px;
            }

            It is more than this margin which is for the same .moodle-dialougue-ft. Or is this in a different place, as the margin is NIL in this CSS rule?

            .moodle-dialogue.chooserdialogue .moodle-dialogue-content .moodle-dialogue-ft {
                margin: 0 0;
            }

            If this is a different rule then the margin here needs to only have one zero can you amend it to read?

            margin: 0;

            I'll test it later.

            Cheers
            Mary

            Show
            lazydaisy Mary Evans added a comment - Actually Andrew it looks OK but cant test it right now as about to have my tea. One thing that jumped out at me is the margin in this line: .moodle-dialogue-base .moodle-dialogue-wrap .moodle-dialogue-ft { margin: 0.7em 1em; text-align: right; background-color: #FFF; font-size: 12px; } It is more than this margin which is for the same .moodle-dialougue-ft. Or is this in a different place, as the margin is NIL in this CSS rule? .moodle-dialogue.chooserdialogue .moodle-dialogue-content .moodle-dialogue-ft { margin: 0 0; } If this is a different rule then the margin here needs to only have one zero can you amend it to read? margin: 0; I'll test it later. Cheers Mary
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Andrew,

            The Notification box looks great, however, there does appear to be a lot of CSS generated by YUI/core/notifications (http://localhost/moodle/theme/yui_combo.php?moodle/1358647156/core/notification/notification.css ) plus just as much again from ( http://localhost/moodle/theme/yui_combo.php?3.8.0/build/widget-base/assets/skins/sam/widget-base.css&3.8.0/build/cssbutton/cssbutton-min.css&3.8.0/build/widget-modality/assets/skins/sam/widget-modality.css&3.8.0/build/widget-stack/assets/skins/sam/widget-stack.css&3.8.0/build/panel/assets/skins/sam/panel.css&moodle/1358647156/core/notification/notification.css ) which the first lot of YUI CSS overwrites.

            which in turn is over writing some of your CSS in theme/base/core.css

            What I think also needs attending to is YUI CSS notifications.css. This should only need to carry the CSS relevant to the Notification dialogue container such as height/width/margin/padding, whilst all the other CSS like background-color/border-color/box-shadow can be inherit from base/core.css

            Also, if in this situation moodle-dialogue is being used as a notification container, then it needs a notification class selector which would be sufficient to carry the CSS to style the moodle-dialogue container.

            Show
            lazydaisy Mary Evans added a comment - - edited Andrew, The Notification box looks great, however, there does appear to be a lot of CSS generated by YUI/core/notifications ( http://localhost/moodle/theme/yui_combo.php?moodle/1358647156/core/notification/notification.css ) plus just as much again from ( http://localhost/moodle/theme/yui_combo.php?3.8.0/build/widget-base/assets/skins/sam/widget-base.css&3.8.0/build/cssbutton/cssbutton-min.css&3.8.0/build/widget-modality/assets/skins/sam/widget-modality.css&3.8.0/build/widget-stack/assets/skins/sam/widget-stack.css&3.8.0/build/panel/assets/skins/sam/panel.css&moodle/1358647156/core/notification/notification.css ) which the first lot of YUI CSS overwrites. which in turn is over writing some of your CSS in theme/base/core.css What I think also needs attending to is YUI CSS notifications.css. This should only need to carry the CSS relevant to the Notification dialogue container such as height/width/margin/padding, whilst all the other CSS like background-color/border-color/box-shadow can be inherit from base/core.css Also, if in this situation moodle-dialogue is being used as a notification container, then it needs a notification class selector which would be sufficient to carry the CSS to style the moodle-dialogue container.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks for the comment Mary,

            Wrath because CSS is not my forte and I'm bound to get bits wrong

            The moodle-dialogue-ft override is because some dialogues will have a footer which ideally need some margin but the chooser dialogue amongst others doesn't. That said, it would probably be a good idea to remove the text-align right as that's something which has leaked through from MDL-35819 really.

            Darn - I'd forgotten about core/notification/notification.css. Your suggestion makes sense to me. I'll try and work on this.

            WRT the core YUI CSS (widget-base, cssbutton-min, etc), this is core YUI CSS and we can't easily and maintainably kill it off (yet). It provides the basics that we then mostly override.

            I'm not 100% sure I understand your last comment:

            Also, if in this situation moodle-dialogue is being used as a notification container, then it needs a notification class selector which would be sufficient to carry the CSS to style the moodle-dialogue container.

            Cheers,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks for the comment Mary, Wrath because CSS is not my forte and I'm bound to get bits wrong The moodle-dialogue-ft override is because some dialogues will have a footer which ideally need some margin but the chooser dialogue amongst others doesn't. That said, it would probably be a good idea to remove the text-align right as that's something which has leaked through from MDL-35819 really. Darn - I'd forgotten about core/notification/notification.css. Your suggestion makes sense to me. I'll try and work on this. WRT the core YUI CSS (widget-base, cssbutton-min, etc), this is core YUI CSS and we can't easily and maintainably kill it off (yet). It provides the basics that we then mostly override. I'm not 100% sure I understand your last comment: Also, if in this situation moodle-dialogue is being used as a notification container, then it needs a notification class selector which would be sufficient to carry the CSS to style the moodle-dialogue container. Cheers, Andrew
            Hide
            dobedobedoh Andrew Nicols added a comment - - edited

            With regards your suggestion:

            Element/Type notification.css base.css
            color / background-*
            text-* / font-* / text-shadow / letter-spacing / line-height
            border-*
            padding / margin
            float
            overflow ?
            cursor
            *-width / *-height

            Will add any others to this list as I come across them. Let me know if you disagree with any.

            Show
            dobedobedoh Andrew Nicols added a comment - - edited With regards your suggestion: Element/Type notification.css base.css color / background-* text-* / font-* / text-shadow / letter-spacing / line-height border-* padding / margin float overflow ? cursor *-width / *-height Will add any others to this list as I come across them. Let me know if you disagree with any.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Mary Evans, I should also note that your solution will also mean we can finally drop all of the !important rules resolving MDL-33584.

            Show
            dobedobedoh Andrew Nicols added a comment - Mary Evans , I should also note that your solution will also mean we can finally drop all of the !important rules resolving MDL-33584 .
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Here's a first bash.
            There is still one !important rule left in and it's against the filter adding gradient. I guess that this can probably be removed..?

            What's the CSS policy when it comes to expansion of rules?
            I guess that within the core themes it's okay to do so since they're minified, but in the YUI module CSS it's less okay (though may become okay if we can get shifter out of the door).

            Show
            dobedobedoh Andrew Nicols added a comment - Here's a first bash. There is still one !important rule left in and it's against the filter adding gradient. I guess that this can probably be removed..? What's the CSS policy when it comes to expansion of rules? I guess that within the core themes it's okay to do so since they're minified, but in the YUI module CSS it's less okay (though may become okay if we can get shifter out of the door).
            Hide
            lazydaisy Mary Evans added a comment -

            If there is a policy attatched to CSS in CORE it must be "anything goes" because nothing is consistent. But to be honest we do try to make it look nice in themes so I guess it is expanded rather than minified. Base tends to be minified too, but I wish it wasn't. It's hard to read and so easy to make a mistake that is hard to find.

            With regards my earlier comment about adding a class CSS selector for notification. What I was meaning is that the test I did during the peer review, was to see if the pop-up widget was styled OK wasn't it? I just thought that since a lot of the CSS was coming via core/notification.css, that it was a notification and so could be styled as such via a single css class selector attached to the dialogue rather than have all that notification CSS file styling the same box twice, three times if you add your CSS to the list. Once is enough if the CSS was in base theme. Then when the CSS is triggered when the notification object appears, then it gets styled as that notification object using base CSS regardless of Theme as Base is the parent theme. It was just a suggestion. It might not work in this case as there is too much CSS styling the same thing. I just don't understand the logic behind YUI CSS, it is too complex for my tiny Gray cells!

            I may have got this all wrong as I am not clear what the relevance of the notification.css is all about. So apologies for this rambling.

            Show
            lazydaisy Mary Evans added a comment - If there is a policy attatched to CSS in CORE it must be "anything goes" because nothing is consistent. But to be honest we do try to make it look nice in themes so I guess it is expanded rather than minified. Base tends to be minified too, but I wish it wasn't. It's hard to read and so easy to make a mistake that is hard to find. With regards my earlier comment about adding a class CSS selector for notification. What I was meaning is that the test I did during the peer review, was to see if the pop-up widget was styled OK wasn't it? I just thought that since a lot of the CSS was coming via core/notification.css, that it was a notification and so could be styled as such via a single css class selector attached to the dialogue rather than have all that notification CSS file styling the same box twice, three times if you add your CSS to the list. Once is enough if the CSS was in base theme. Then when the CSS is triggered when the notification object appears, then it gets styled as that notification object using base CSS regardless of Theme as Base is the parent theme. It was just a suggestion. It might not work in this case as there is too much CSS styling the same thing. I just don't understand the logic behind YUI CSS, it is too complex for my tiny Gray cells! I may have got this all wrong as I am not clear what the relevance of the notification.css is all about. So apologies for this rambling.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            There's not much policy at present I don't think - Dan Poltawski opened MDLSITE-1796 to address this.

            The M.core.dialogue is actually part of a series of dialogues including warnings, alerts, and other types of notifications which are all defined in /lib/yui/notification/notification.js. The CSS which goes with them is in /lib/yui/notification/assets/skins/sam/notification.css (yeah... because that's obvious). To retrieve that CSS, we use the YUI combo loader and point it at /moodle/<themerev>/core/notification.css and it does the magic to get the correct file.

            On top of that, the core YUI components which we extend also have their own styling and these are included too (but mostly overridden).

            I think that we have two options here:

            1. keep the sizing separate to the styling by putting the height/width/etc into notification.css and the style (colors, etc) into the base theme; or
            2. ditch notification.css and move it all into the base theme.

            I'm really in two minds here. On the one hand the base theme seems like the ideal place to put it because themes typically extend the base theme. On the other, the base theme adds too much control for some themes and those themes wish to do things from scratch.

            The more I think about it though, the more I think that this should go into the base theme and we can then ditch notification.css. If a theme does not wish to extend base, then they'll need to be aware that they need to handle dialogue styling and dialogue sizing. Ditching notification.css would also remove some of the complexity, and reduce the number of pages we load.

            I'd love to have your opinion on this though.

            Show
            dobedobedoh Andrew Nicols added a comment - There's not much policy at present I don't think - Dan Poltawski opened MDLSITE-1796 to address this. The M.core.dialogue is actually part of a series of dialogues including warnings, alerts, and other types of notifications which are all defined in /lib/yui/notification/notification.js. The CSS which goes with them is in /lib/yui/notification/assets/skins/sam/notification.css (yeah... because that's obvious). To retrieve that CSS, we use the YUI combo loader and point it at /moodle/<themerev>/core/notification.css and it does the magic to get the correct file. On top of that, the core YUI components which we extend also have their own styling and these are included too (but mostly overridden). I think that we have two options here: keep the sizing separate to the styling by putting the height/width/etc into notification.css and the style (colors, etc) into the base theme; or ditch notification.css and move it all into the base theme. I'm really in two minds here. On the one hand the base theme seems like the ideal place to put it because themes typically extend the base theme. On the other, the base theme adds too much control for some themes and those themes wish to do things from scratch. The more I think about it though, the more I think that this should go into the base theme and we can then ditch notification.css. If a theme does not wish to extend base, then they'll need to be aware that they need to handle dialogue styling and dialogue sizing. Ditching notification.css would also remove some of the complexity, and reduce the number of pages we load. I'd love to have your opinion on this though.
            Hide
            lazydaisy Mary Evans added a comment -

            @Andrew Nicols

            I think adding notification.css into base would be a good move if it means we can lessen the amount of CSS processed by YUI in Moodle. We are finding more and more that YUI is duplicating CSS as I found while testing this tracker issue.

            Show
            lazydaisy Mary Evans added a comment - @ Andrew Nicols I think adding notification.css into base would be a good move if it means we can lessen the amount of CSS processed by YUI in Moodle. We are finding more and more that YUI is duplicating CSS as I found while testing this tracker issue.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Voila - how's that? I've just pushed an updated branch which entirely removes notification.css and places all of its rules into theme/base/style/core.css.

            Show
            dobedobedoh Andrew Nicols added a comment - Voila - how's that? I've just pushed an updated branch which entirely removes notification.css and places all of its rules into theme/base/style/core.css.
            Hide
            lazydaisy Mary Evans added a comment -

            +1 from me!

            By the way I tested the .dir-rtl CSS fix for the location of the close button, but it did not work when I tested this originally. I spent a good half hour trying to figure out what was stopping it.

            I'll take a better look at it with these new changes in place.

            Show
            lazydaisy Mary Evans added a comment - +1 from me! By the way I tested the .dir-rtl CSS fix for the location of the close button, but it did not work when I tested this originally. I spent a good half hour trying to figure out what was stopping it. I'll take a better look at it with these new changes in place.
            Hide
            lazydaisy Mary Evans added a comment -

            Just tested it and looks great. Only problem is dir-rtl close button needs fixing. Can you add this CSS?

            .dir-rtl .yui3-panel .yui3-widget-hd .yui3-widget-buttons {
                position: absolute;
                left: 0;
                top: 0;
            }

            Also a z-index issue for themes like Anomaly and Afterburner

            div.moodle-dialogue-base div.yui3-widget{
                z-index: 600!important;
            }

            Show
            lazydaisy Mary Evans added a comment - Just tested it and looks great. Only problem is dir-rtl close button needs fixing. Can you add this CSS? .dir-rtl .yui3-panel .yui3-widget-hd .yui3-widget-buttons { position: absolute; left: 0; top: 0; } Also a z-index issue for themes like Anomaly and Afterburner div.moodle-dialogue-base div.yui3-widget{ z-index: 600!important; }
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks Mary,

            I've added the relevant CSS - not using quite the selectors you suggest but the equivalent ones we're already using.

            Will submit for Integration.

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks Mary, I've added the relevant CSS - not using quite the selectors you suggest but the equivalent ones we're already using. Will submit for Integration.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Hi Andrew,

            I've added the relevant CSS - not using quite the selectors you suggest but the equivalent ones we're already using.

            That may be so but the .dir.rtl CSS for the closebutton does not work without the selectors I added, and I added them for a reason. CSS does not work the way you think especially as YUI is laced with selector IDs which take precedence over class selectors.

            Until you do away with YUI you have to use YUI CSS. This is one of the theme designer's bug bears. Fact is we have got used to it now.

            Anyway I can fix that one after this is integrated.

            Show
            lazydaisy Mary Evans added a comment - - edited Hi Andrew, I've added the relevant CSS - not using quite the selectors you suggest but the equivalent ones we're already using. That may be so but the .dir.rtl CSS for the closebutton does not work without the selectors I added, and I added them for a reason. CSS does not work the way you think especially as YUI is laced with selector IDs which take precedence over class selectors. Until you do away with YUI you have to use YUI CSS. This is one of the theme designer's bug bears. Fact is we have got used to it now. Anyway I can fix that one after this is integrated.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Mary,

            Looking at the CSS and generated output, the reason why the closebutton would have been on the right in RTL wasn't because of non-absolute positioning, but because it had a right value of 0 with no left value.

            Applying a left value of 0 (as well as a right value of 0) means that the span takes up the entire width of the titlebar. If this is the intended behaviour then it should happen in LTR too. In my opinion it's an incorrect behaviour and the correct action is to unset the right value when in RTL.

            I've adjusted the patch slightly to apply a left value of 0, and a right value of auto. This is done on a more-specific selector which matches the other selectors which we use. If we don't use the moodle selectors then we risk making it more confusing in the CSS with multiple selectors in the same file doing competing things to the same element.

            As an example, we're already applying CSS for:

            .moodle-dialogue-base .moodle-dialogue-wrap .moodle-dialogue-hd .yui3-widget-buttons

            IMO it would be confusing to also the following in the same CSS file:

            .dir-rtl .yui3-panel .yui3-widget-hd .yui3-widget-buttons

            When in reality, we can use:

            .dir-rtl .moodle-dialogue-base .moodle-dialogue-wrap .moodle-dialogue-hd .yui3-widget-buttons

            Similarly, we're already using:

            .moodle-dialogue-base .moodle-dialogue

            Which has the same precedence as:

            div.moodle-dialogue-base div.yui3-widget

            I've put this back into development until we can resolve this,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Mary, Looking at the CSS and generated output, the reason why the closebutton would have been on the right in RTL wasn't because of non-absolute positioning, but because it had a right value of 0 with no left value. Applying a left value of 0 (as well as a right value of 0) means that the span takes up the entire width of the titlebar. If this is the intended behaviour then it should happen in LTR too. In my opinion it's an incorrect behaviour and the correct action is to unset the right value when in RTL. I've adjusted the patch slightly to apply a left value of 0, and a right value of auto. This is done on a more-specific selector which matches the other selectors which we use. If we don't use the moodle selectors then we risk making it more confusing in the CSS with multiple selectors in the same file doing competing things to the same element. As an example, we're already applying CSS for: .moodle-dialogue-base .moodle-dialogue-wrap .moodle-dialogue-hd .yui3-widget-buttons IMO it would be confusing to also the following in the same CSS file: .dir-rtl .yui3-panel .yui3-widget-hd .yui3-widget-buttons When in reality, we can use: .dir-rtl .moodle-dialogue-base .moodle-dialogue-wrap .moodle-dialogue-hd .yui3-widget-buttons Similarly, we're already using: .moodle-dialogue-base .moodle-dialogue Which has the same precedence as: div.moodle-dialogue-base div.yui3-widget I've put this back into development until we can resolve this, Andrew
            Hide
            lazydaisy Mary Evans added a comment -

            Remove the .dir-rtl for that closebutton CSS and I'll file a new issue to fix it. Now that I can see the way you are doing this makes it easier to understand. So thanks for explaining it, it makes a lot of sense.

            Show
            lazydaisy Mary Evans added a comment - Remove the .dir-rtl for that closebutton CSS and I'll file a new issue to fix it. Now that I can see the way you are doing this makes it easier to understand. So thanks for explaining it, it makes a lot of sense.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Adding Nadav Kavalerchik who is our resident RTL CSS expert.

            Nadav, if you have a moment to spare can you comment here as I am wondering now if we even need to change the direction of the "close button" in the Moodle dialogue box from right to left.

            Thanks
            Mary

            Show
            lazydaisy Mary Evans added a comment - - edited Adding Nadav Kavalerchik who is our resident RTL CSS expert. Nadav, if you have a moment to spare can you comment here as I am wondering now if we even need to change the direction of the "close button" in the Moodle dialogue box from right to left. Thanks Mary
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Just to check, which dir-rtl closebutton rule did you want me to remove?

            Show
            dobedobedoh Andrew Nicols added a comment - Just to check, which dir-rtl closebutton rule did you want me to remove?
            Hide
            lazydaisy Mary Evans added a comment -

            Sorry I did not see you had added the revised CSS for .dir-rtl, in which case you can leave that in.

            Proceed as planned!

            Show
            lazydaisy Mary Evans added a comment - Sorry I did not see you had added the revised CSS for .dir-rtl, in which case you can leave that in. Proceed as planned!
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Resubmitting for integration

            Show
            dobedobedoh Andrew Nicols added a comment - Resubmitting for integration
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Thanks Mary.

            In general, the "Close" icon should switch sides, when in RTL mode.
            otherwise the Title's text (which has CSS roles for switching sides) will be override by the Icon.
            (We already fixed some issues like this one in the past. Moodle 2.1 or 2.2. as far as I remember)

            A nice concept would be to center the Title's text, which will help in cases where someone forgets to handle the RTL side switch.

            Show
            nadavkav Nadav Kavalerchik added a comment - Thanks Mary. In general, the "Close" icon should switch sides, when in RTL mode. otherwise the Title's text (which has CSS roles for switching sides) will be override by the Icon. (We already fixed some issues like this one in the past. Moodle 2.1 or 2.2. as far as I remember) A nice concept would be to center the Title's text, which will help in cases where someone forgets to handle the RTL side switch.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Nadav,

            One of the things that this does is to centre the dialogue titles.

            I've attached some screenshots to demonstrate how it looks in RTL with these changes.

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Nadav, One of the things that this does is to centre the dialogue titles. I've attached some screenshots to demonstrate how it looks in RTL with these changes. Andrew
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sorry, Just realised that the help screenshot I included is actually from MDL-35819. Otherwise it's still valid.

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry, Just realised that the help screenshot I included is actually from MDL-35819 . Otherwise it's still valid.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Andrew,
            Best

            And btw, Beautiful UI and great move!

            Show
            nadavkav Nadav Kavalerchik added a comment - Andrew, Best And btw, Beautiful UI and great move!
            Hide
            lazydaisy Mary Evans added a comment -

            Here is a screenshot of the Moodle dialogue notification in Afterburner in Hebrew
            which I have just taken a few minutes ago.

            It's looking good!

            Show
            lazydaisy Mary Evans added a comment - Here is a screenshot of the Moodle dialogue notification in Afterburner in Hebrew which I have just taken a few minutes ago. It's looking good!
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Just closed MDL-33584 as it was redundant, and no longer required.

            In checking the CSS in this fix I found these .dir-rtl css rules...
            the first pair can be deleted as they are duplicated in filemanager.css

            .dir-rtl .file-picker .yui-layout-unit-left {left:500px !important;}
            .dir-rtl .file-picker .yui-layout-unit-center {left:0px !important;}
            

            Not sure about these. It could be they need modifying to match moodle dialogue selectors?

            .dir-rtl.yui-skin-sam .yui-panel .hd {text-align:left;}
            .dir-rtl .yui-skin-sam .yui-layout .yui-layout-unit div.yui-layout-bd {
            text-align:right;
            }
            

            Show
            lazydaisy Mary Evans added a comment - - edited Just closed MDL-33584 as it was redundant, and no longer required. In checking the CSS in this fix I found these .dir-rtl css rules... the first pair can be deleted as they are duplicated in filemanager.css .dir-rtl .file-picker .yui-layout-unit-left {left:500px !important;} .dir-rtl .file-picker .yui-layout-unit-center {left:0px !important;} Not sure about these. It could be they need modifying to match moodle dialogue selectors? .dir-rtl.yui-skin-sam .yui-panel .hd {text-align:left;} .dir-rtl .yui-skin-sam .yui-layout .yui-layout-unit div.yui-layout-bd { text-align:right; }
            Hide
            lazydaisy Mary Evans added a comment -

            As an aside, Andrew, I think it may be a better idea, and easier to manage, if you were to create a separate style-sheet called dialogue.css and transfer all the moodle dialogue css from core.css to it. Much the same as was done with filemanager.css.

            What do you think?

            Show
            lazydaisy Mary Evans added a comment - As an aside, Andrew, I think it may be a better idea, and easier to manage, if you were to create a separate style-sheet called dialogue.css and transfer all the moodle dialogue css from core.css to it. Much the same as was done with filemanager.css. What do you think?
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I think that may be a good idea. It would certainly make it easier for theme designers to find all of the code relating to YUI dialogues, and panels in order to override certain aspects.

            Show
            dobedobedoh Andrew Nicols added a comment - I think that may be a good idea. It would certainly make it easier for theme designers to find all of the code relating to YUI dialogues, and panels in order to override certain aspects.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            The latter two rules with .yui-skin-sam are a different matter. In YUI3 the CSS namespace changed from yui to yui3. So a YUI2 panel as .yui-panel whilst a YUI3 panel is .yui3-panel. These will only therefore affect YUI2 panels.

            We're trying to wipe out those locations still using YUI2. The ones I can think of off the top of my head are:

            • question bank chooser
            • SCORM navigation panel

            Hmmm.. I'm sure that there are more!

            Either way, I think that we should leave these where they are for the moment and avoid touching them without knowing more about where they are actually used.

            I think that filemanager CSS is probably best in another issue rather than tacked onto this one.

            Show
            dobedobedoh Andrew Nicols added a comment - The latter two rules with .yui-skin-sam are a different matter. In YUI3 the CSS namespace changed from yui to yui3. So a YUI2 panel as .yui-panel whilst a YUI3 panel is .yui3-panel. These will only therefore affect YUI2 panels. We're trying to wipe out those locations still using YUI2. The ones I can think of off the top of my head are: question bank chooser SCORM navigation panel Hmmm.. I'm sure that there are more! Either way, I think that we should leave these where they are for the moment and avoid touching them without knowing more about where they are actually used. I think that filemanager CSS is probably best in another issue rather than tacked onto this one.
            Hide
            lazydaisy Mary Evans added a comment -

            I'll set up a tracker so I can delete those two duplicated .dir-rtl css rules and fix them now before I forget!

            Show
            lazydaisy Mary Evans added a comment - I'll set up a tracker so I can delete those two duplicated .dir-rtl css rules and fix them now before I forget!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Rebased on latest master.

            Show
            dobedobedoh Andrew Nicols added a comment - Rebased on latest master.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master. Thanks Andrew.

            Show
            poltawski Dan Poltawski added a comment - Integrated to master. Thanks Andrew.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sorry guys, whilst working on another issue I noticed that I'd missed some of the trailing semi-colons on some CSS rules.

            I've put a fix for this on the same branch as before:
            git://github.org/andrewnicols/moodle.git
            MDL-37645-m
            https://github.com/andrewnicols/moodle/commit/2c653f

            Commit hash is 2c653f

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry guys, whilst working on another issue I noticed that I'd missed some of the trailing semi-colons on some CSS rules. I've put a fix for this on the same branch as before: git://github.org/andrewnicols/moodle.git MDL-37645 -m https://github.com/andrewnicols/moodle/commit/2c653f Commit hash is 2c653f
            Hide
            poltawski Dan Poltawski added a comment -

            I've pulled that fix in (github.com btw )

            Show
            poltawski Dan Poltawski added a comment - I've pulled that fix in (github.com btw )
            Hide
            fred Frédéric Massart added a comment -

            Failing to confirm that the behaviour is expected. The popups appear nice on both the activity chooser and the exception message. But the exception message is empty, and if I move the exception down under the inclusion of config.php, then nothing happens at all. Is that normal? How can I see an exception message with some content?

            Show
            fred Frédéric Massart added a comment - Failing to confirm that the behaviour is expected. The popups appear nice on both the activity chooser and the exception message. But the exception message is empty, and if I move the exception down under the inclusion of config.php, then nothing happens at all. Is that normal? How can I see an exception message with some content?
            Hide
            lazydaisy Mary Evans added a comment -

            Andrew, looks like you missed one in the ones you caught!

            Line 1010:
            .moodle-dialogue-exception .param-stacktrace .stacktrace-file

            display:inline-block

            As can be seen here...
            https://github.com/andrewnicols/moodle/commit/MDL-37645-m

            Show
            lazydaisy Mary Evans added a comment - Andrew, looks like you missed one in the ones you caught! Line 1010: .moodle-dialogue-exception .param-stacktrace .stacktrace-file display:inline-block As can be seen here... https://github.com/andrewnicols/moodle/commit/MDL-37645-m
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Mary, i've pushed da fix to that one too. (grr!)

            Show
            poltawski Dan Poltawski added a comment - Thanks Mary, i've pushed da fix to that one too. (grr!)
            Hide
            lazydaisy Mary Evans added a comment - - edited

            @ Andrew, I tend to use this...
            http://jigsaw.w3.org/css-validator/

            You will find it helpful when checking CSS, I use the upload file method. It does not like -web-kit and -moz and IE hacks...so be warned!

            But you will find it invaluable when doing these sort of fixes.

            Show
            lazydaisy Mary Evans added a comment - - edited @ Andrew, I tend to use this... http://jigsaw.w3.org/css-validator/ You will find it helpful when checking CSS, I use the upload file method. It does not like -web-kit and -moz and IE hacks...so be warned! But you will find it invaluable when doing these sort of fixes.
            Hide
            poltawski Dan Poltawski added a comment -

            PING!

            Show
            poltawski Dan Poltawski added a comment - PING!
            Hide
            lazydaisy Mary Evans added a comment -

            What's gone wrong now?

            Show
            lazydaisy Mary Evans added a comment - What's gone wrong now?
            Hide
            poltawski Dan Poltawski added a comment -

            Fred wrote:

            Failing to confirm that the behaviour is expected. The popups appear nice on both the activity chooser and the exception message. But the exception message is empty, and if I move the exception down under the inclusion of config.php, then nothing happens at all. Is that normal? How can I see an exception message with some content?

            Show
            poltawski Dan Poltawski added a comment - Fred wrote: Failing to confirm that the behaviour is expected. The popups appear nice on both the activity chooser and the exception message. But the exception message is empty, and if I move the exception down under the inclusion of config.php, then nothing happens at all. Is that normal? How can I see an exception message with some content?
            Hide
            lazydaisy Mary Evans added a comment -

            I did not get a stack trace as the but there was some writing in the top see the image I posted, the one with the Afterburner theme in the b/ground.

            I'll test it again and see if something changed.

            Show
            lazydaisy Mary Evans added a comment - I did not get a stack trace as the but there was some writing in the top see the image I posted, the one with the Afterburner theme in the b/ground. I'll test it again and see if something changed.
            Hide
            lazydaisy Mary Evans added a comment -

            I am getting this Error:

            File: http://localhost/moodle/theme/yui_combo.php?3.8.0/build/simpleyui/simpleyui.js&3.8.0/build/loader/loader.js
            Line: 18226

            Perhaps there IS a problem, I was just assuming it was what was expected.

            Show
            lazydaisy Mary Evans added a comment - I am getting this Error: File: http://localhost/moodle/theme/yui_combo.php?3.8.0/build/simpleyui/simpleyui.js&3.8.0/build/loader/loader.js Line: 18226 Perhaps there IS a problem, I was just assuming it was what was expected.
            Hide
            poltawski Dan Poltawski added a comment -

            Just had a look at this in a bit more detail, it seems to me that the testing instructions are rubbish.

            The use of moodle_exception before config.php is included is obviously a bit broken. Furthermore, adding it after config.php seems to do nothing, like Fred says. So we don't have any good way to test this properly. Especially demonstrating the use of the stack trace.

            However, from what I can tell this is the same on moodle.git, so its not a new regression.

            Show
            poltawski Dan Poltawski added a comment - Just had a look at this in a bit more detail, it seems to me that the testing instructions are rubbish. The use of moodle_exception before config.php is included is obviously a bit broken. Furthermore, adding it after config.php seems to do nothing, like Fred says. So we don't have any good way to test this properly. Especially demonstrating the use of the stack trace. However, from what I can tell this is the same on moodle.git, so its not a new regression.
            Hide
            poltawski Dan Poltawski added a comment - - edited

            This demonstrates the use of the stack trace:
            diff --git a/enrol/manual/ajax.php b/enrol/manual/ajax.php
            index 093ee05..74a3f06 100644
            — a/enrol/manual/ajax.php
            +++ b/enrol/manual/ajax.php
            @@ -28,6 +28,8 @@
            define('AJAX_SCRIPT', true);

            require('../../config.php');
            +
            +throw new moodle_exception('sdfs');
            require_once($CFG->dirroot.'/enrol/locallib.php');
            require_once($CFG->dirroot.'/group/lib.php');

            Show
            poltawski Dan Poltawski added a comment - - edited This demonstrates the use of the stack trace: diff --git a/enrol/manual/ajax.php b/enrol/manual/ajax.php index 093ee05..74a3f06 100644 — a/enrol/manual/ajax.php +++ b/enrol/manual/ajax.php @@ -28,6 +28,8 @@ define('AJAX_SCRIPT', true); require('../../config.php'); + +throw new moodle_exception('sdfs'); require_once($CFG->dirroot.'/enrol/locallib.php'); require_once($CFG->dirroot.'/group/lib.php');
            Hide
            poltawski Dan Poltawski added a comment -

            Which demonstrates that the stack trace is a bit broken.

            Show
            poltawski Dan Poltawski added a comment - Which demonstrates that the stack trace is a bit broken.
            Hide
            poltawski Dan Poltawski added a comment -

            Attached a screenshot of the stacktrace. Its not visible, isn't being wrapped, is very hard to copy and paste the whole content.

            Show
            poltawski Dan Poltawski added a comment - Attached a screenshot of the stacktrace. Its not visible, isn't being wrapped, is very hard to copy and paste the whole content.
            Hide
            poltawski Dan Poltawski added a comment -

            Lol, how did I manage to assign this to myself.

            Show
            poltawski Dan Poltawski added a comment - Lol, how did I manage to assign this to myself.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sorry for missing your comment Frédéric Massart, not sure how I did so!

            Sorry yes, the testing instructions were wrong, it should have been after the inclusion of config.php as Dan worked out.

            Although it isn't wrapped, this isn't a regression from previous behaviour. I should also point out that the stacktrace is only shown when M.cfg.developerdebug is set. We do have a couple of options to improve this:

            • convert the stacktrace field to a textarea to allow copy/paste more easily
            • make the dialogue wider when in debugging

            Either way, I think that both of the above are separate issues.

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry for missing your comment Frédéric Massart , not sure how I did so! Sorry yes, the testing instructions were wrong, it should have been after the inclusion of config.php as Dan worked out. Although it isn't wrapped, this isn't a regression from previous behaviour. I should also point out that the stacktrace is only shown when M.cfg.developerdebug is set. We do have a couple of options to improve this: convert the stacktrace field to a textarea to allow copy/paste more easily make the dialogue wider when in debugging Either way, I think that both of the above are separate issues.
            Hide
            poltawski Dan Poltawski added a comment -

            Well, it wasn't just after the inclusion of config.php, it was also in a different file.

            Show
            poltawski Dan Poltawski added a comment - Well, it wasn't just after the inclusion of config.php, it was also in a different file.
            Hide
            poltawski Dan Poltawski added a comment -

            In any case, i'm passing this.

            Show
            poltawski Dan Poltawski added a comment - In any case, i'm passing this.
            Hide
            damyon Damyon Wiese added a comment -

            Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

            Regards, Damyon

            Show
            damyon Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon
            Hide
            lazydaisy Mary Evans added a comment - - edited

            So pleased this got through...eventually.
            I did not say anything, but I noticed a small error in the CSS which is not causing problems, but to help keep things tidy and may even help remove an !important tag too, I'll open a new tracker to fix it.

            Cheers and thanks.
            Mary

            Show
            lazydaisy Mary Evans added a comment - - edited So pleased this got through...eventually. I did not say anything, but I noticed a small error in the CSS which is not causing problems, but to help keep things tidy and may even help remove an !important tag too, I'll open a new tracker to fix it. Cheers and thanks. Mary

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13