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:
    • Rank:
      47338

      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.

        Issue Links

          Activity

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

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

          Show
          Mary Evans added a comment - Why wrath? Do you feel like the Sword of Damocles is over you or something? LOL
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Andrew Nicols added a comment -

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

          Show
          Andrew Nicols added a comment - Just to check, which dir-rtl closebutton rule did you want me to remove?
          Hide
          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
          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
          Andrew Nicols added a comment -

          Resubmitting for integration

          Show
          Andrew Nicols added a comment - Resubmitting for integration
          Hide
          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
          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
          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
          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
          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
          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
          Nadav Kavalerchik added a comment -

          Andrew,
          Best

          And btw, Beautiful UI and great move!

          Show
          Nadav Kavalerchik added a comment - Andrew, Best And btw, Beautiful UI and great move!
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Andrew Nicols added a comment -

          Rebased on latest master.

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

          Integrated to master. Thanks Andrew.

          Show
          Dan Poltawski added a comment - Integrated to master. Thanks Andrew.
          Hide
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - I've pulled that fix in (github.com btw )
          Hide
          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
          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
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - Thanks Mary, i've pushed da fix to that one too. (grr!)
          Hide
          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
          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
          Dan Poltawski added a comment -

          PING!

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

          What's gone wrong now?

          Show
          Mary Evans added a comment - What's gone wrong now?
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Dan Poltawski added a comment -

          Which demonstrates that the stack trace is a bit broken.

          Show
          Dan Poltawski added a comment - Which demonstrates that the stack trace is a bit broken.
          Hide
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - Lol, how did I manage to assign this to myself.
          Hide
          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
          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
          Dan Poltawski added a comment -

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

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

          In any case, i'm passing this.

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

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

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon
          Hide
          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
          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: