Moodle
  1. Moodle
  2. MDL-39814

Improve editing icons for usability on all types of screens

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Run unit tests.
      2. Run behat tests (js tests must be run).
      3. Log in as an admin.
      4. Browse to a course and turn on editing.
      5. Test all of the editing options shown for both activities and resources.
      6. Test all of the editing options shown for blocks.
      7. Disable JS.
      8. Check that check that the display is passable.
      9. Re-enable JS.
      10. Browse to the admin notifications page.
      11. Re-test block editing actions.
      • Force the group mode for the course and then view the course with editing on to make sure the "fixed" group icons are OK.
      • Test in both standard + clean and if want go the extra mile and test in other themes.
      • Toggle availability and completion settings to make sure alignment works with all combinations.
      • Toggle the two settings modeditingmenu and blockeditingmenu and check they return the menu to its former state.
      Show
      Run unit tests. Run behat tests (js tests must be run). Log in as an admin. Browse to a course and turn on editing. Test all of the editing options shown for both activities and resources. Test all of the editing options shown for blocks. Disable JS. Check that check that the display is passable. Re-enable JS. Browse to the admin notifications page. Re-test block editing actions. Force the group mode for the course and then view the course with editing on to make sure the "fixed" group icons are OK. Test in both standard + clean and if want go the extra mile and test in other themes. Toggle availability and completion settings to make sure alignment works with all combinations. Toggle the two settings modeditingmenu and blockeditingmenu and check they return the menu to its former state.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      wip-MDL-39814-m26
    • Story Points:
      40
    • Rank:
      147
    • Sprint:
      FRONTEND Sprint 2

      Description

      Editing icons are sometimes messy and confusing, as well as being small and hard to hit on mobile platforms.

      This issue is about finding a solution that simplifies the display a lot by reducing available options, while trying not to slow things down for power users who are heavily engaged in creating/editing/moving/configuring activities.

      It needs to work on smartphones as well as on large desktops.

      There may be user preferences involved.

        Issue Links

          Activity

          Hide
          moodle.com added a comment -

          small collections of icons, such as those on block controls, or editing an activity, could be replaced with a single larger icon that displays a menu with text links to the other icons.

          Show
          moodle.com added a comment - small collections of icons, such as those on block controls, or editing an activity, could be replaced with a single larger icon that displays a menu with text links to the other icons.
          Hide
          Damyon Wiese added a comment -

          Starting to look at this:

          Some decisions need to be made about when to use this new menu (only for touchscreens - always etc).

          Show
          Damyon Wiese added a comment - Starting to look at this: Some decisions need to be made about when to use this new menu (only for touchscreens - always etc).
          Hide
          David Scotson added a comment -

          Can I suggest that drop-down menus, while ugly and probably to be avoided on desktops, can be very handy for presenting options to users on small devices in a native way without mucking around with finicky js. An option to consider at least.

          Show
          David Scotson added a comment - Can I suggest that drop-down menus, while ugly and probably to be avoided on desktops, can be very handy for presenting options to users on small devices in a native way without mucking around with finicky js. An option to consider at least.
          Hide
          Damyon Wiese added a comment -

          Thanks David,

          I have almost finished an initial version of this using js for the dropdowns. I agree that the native select lists on mobile have good features, especially for long lists, in this case though - we want to keep the icons + text together and there are only a small number of items in the list.

          Show
          Damyon Wiese added a comment - Thanks David, I have almost finished an initial version of this using js for the dropdowns. I agree that the native select lists on mobile have good features, especially for long lists, in this case though - we want to keep the icons + text together and there are only a small number of items in the list.
          Hide
          Damyon Wiese added a comment -

          Note - rescaled issue thinking carefully about how much work I have done and how much is left.

          Quick summary of progress:

          Prototype of this working with "clean" theme only is impemented on the branch. The changes are to split the list of action icons into "status" and "action" icons - the status icons are always displayed but with more padding. This includes things like the drag/drop handles which can't really be put in a menu. Everything else goes in a new actionmenu with text + icon. This applies to the block menus as well.

          Still TODO on this branch:

          1. fix visibility on the blocks menu (needs changes to find the block that triggered the action)
          2. code cleanup - add a new class to encapsulate adding an actionmenuitem to the list (is it status, or action, text + icon). The code for getting lists of actions for sections/blocks should then be much simpler.
          3. css tidy + add changes to base theme
          Show
          Damyon Wiese added a comment - Note - rescaled issue thinking carefully about how much work I have done and how much is left. Quick summary of progress: Prototype of this working with "clean" theme only is impemented on the branch. The changes are to split the list of action icons into "status" and "action" icons - the status icons are always displayed but with more padding. This includes things like the drag/drop handles which can't really be put in a menu. Everything else goes in a new actionmenu with text + icon. This applies to the block menus as well. Still TODO on this branch: fix visibility on the blocks menu (needs changes to find the block that triggered the action) code cleanup - add a new class to encapsulate adding an actionmenuitem to the list (is it status, or action, text + icon). The code for getting lists of actions for sections/blocks should then be much simpler. css tidy + add changes to base theme
          Hide
          Damyon Wiese added a comment -

          Unassigning myself so someone else can pick this up and finish it off (I'm back on integration for 3 weeks now).

          Show
          Damyon Wiese added a comment - Unassigning myself so someone else can pick this up and finish it off (I'm back on integration for 3 weeks now).
          Hide
          Sam Hemelryk added a comment -

          I'll pick this up seeing as I'm off integration for this sprint

          Show
          Sam Hemelryk added a comment - I'll pick this up seeing as I'm off integration for this sprint
          Hide
          Damyon Wiese added a comment -

          Thanks Sam, feel free to ping me if you have questions.

          Some notes on the design:

          I played with floating the icons right - but they look odd next to the completion icons which are a different size. I also tested out a drop down arrow next to the menu icon to make it more obvious, but after talking with barbara, I took that back out.

          Show
          Damyon Wiese added a comment - Thanks Sam, feel free to ping me if you have questions. Some notes on the design: I played with floating the icons right - but they look odd next to the completion icons which are a different size. I also tested out a drop down arrow next to the menu icon to make it more obvious, but after talking with barbara, I took that back out.
          Hide
          Sam Hemelryk added a comment -

          Ok time for this to go up for peer-review.

          Damyon, not sure how your time stands at the moment but if you could take a look at this I'd really appreciate it.
          It wouldn't have to be a peer-reviewer but a look to the end result to see if it matches discussions you've had.

          Things to note:

          1. Course and block editing actions have both been improved.
          2. This adds a new component action_menu
            • Primary (always displayed items) and secondary items displayed in a drop down.
            • renderable by the core_renderer.
            • a new shifted YUI module:
              • Accessibility has been addressed.
              • Uses the existing notifications module.
              • Minimal event impact (2 delegated events to control all menus on the page)
          3. Greatly modifies course editings resource toolbox:
            • Much less dependent upon the DOM. Data persistence through properties rather than DOM structure traversal. (makes it much more robust)
            • Reduced the event footprint, all course module AJAX is handled by a single delegated event for all modules on the page.
            • Reorganised methods to address scoping issues.
            • YUIDoc syntax used. It will be easy to move this to a YUI shifted module now, we should do that in the future.
          4. CSS styles added to both base and bootstrapbase.
          5. RTL supported as best we can think it should be.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Ok time for this to go up for peer-review. Damyon, not sure how your time stands at the moment but if you could take a look at this I'd really appreciate it. It wouldn't have to be a peer-reviewer but a look to the end result to see if it matches discussions you've had. Things to note: Course and block editing actions have both been improved. This adds a new component action_menu Primary (always displayed items) and secondary items displayed in a drop down. renderable by the core_renderer. a new shifted YUI module: Accessibility has been addressed. Uses the existing notifications module. Minimal event impact (2 delegated events to control all menus on the page) Greatly modifies course editings resource toolbox: Much less dependent upon the DOM. Data persistence through properties rather than DOM structure traversal. (makes it much more robust) Reduced the event footprint, all course module AJAX is handled by a single delegated event for all modules on the page. Reorganised methods to address scoping issues. YUIDoc syntax used. It will be easy to move this to a YUI shifted module now, we should do that in the future. CSS styles added to both base and bootstrapbase. RTL supported as best we can think it should be. Many thanks Sam
          Hide
          Damyon Wiese added a comment -

          Thanks Sam, will take a look now.

          Show
          Damyon Wiese added a comment - Thanks Sam, will take a look now.
          Hide
          Damyon Wiese added a comment -

          Just documenting some suggestions from Martin:

          • change the menu icon a bit when the menu is open so you can see which activity it relates to (or some other indication)
          • increase size/padding for remaining icons (too hard for touch)
          • later - not in this issue - maybe look at removing editing mode completely
          Show
          Damyon Wiese added a comment - Just documenting some suggestions from Martin: change the menu icon a bit when the menu is open so you can see which activity it relates to (or some other indication) increase size/padding for remaining icons (too hard for touch) later - not in this issue - maybe look at removing editing mode completely
          Hide
          Damyon Wiese added a comment -

          One suggestion from checking the code:

          Right now actionmenu is a renderable that takes a list of actionlinks - I think it should take a list of actionmenuitems that has a flag for the information about whether something can be put in the menu or not. Having a separate class for that would be more explicit (relying on the status class was a hack that I added - but I don't think we should keep in the final code). Having a separate renderable for this could reduce alot of the code e.g. in course_get_cm_edit_actions().

          Show
          Damyon Wiese added a comment - One suggestion from checking the code: Right now actionmenu is a renderable that takes a list of actionlinks - I think it should take a list of actionmenuitems that has a flag for the information about whether something can be put in the menu or not. Having a separate class for that would be more explicit (relying on the status class was a hack that I added - but I don't think we should keep in the final code). Having a separate renderable for this could reduce alot of the code e.g. in course_get_cm_edit_actions().
          Hide
          Damyon Wiese added a comment -

          Raj just had a look and his suggestions were:

          1 - open the menus to the right, not the left (I don't necessarily agree here)
          2 - move the "move" icon to the left of the activity name (similar to how move works elsewhere, e.g. moving courses in myhome or drag and drop in the lesson - I'll add a new issue for this
          3 - important - when using the keyboard, the menu is not the next thing in the tab order once it is open - it is the last thing in the page in the tab order.
          4 - take the move left/right out of the menu (I don't agree with this one).

          Show
          Damyon Wiese added a comment - Raj just had a look and his suggestions were: 1 - open the menus to the right, not the left (I don't necessarily agree here) 2 - move the "move" icon to the left of the activity name (similar to how move works elsewhere, e.g. moving courses in myhome or drag and drop in the lesson - I'll add a new issue for this 3 - important - when using the keyboard, the menu is not the next thing in the tab order once it is open - it is the last thing in the page in the tab order. 4 - take the move left/right out of the menu (I don't agree with this one).
          Hide
          Damyon Wiese added a comment -

          Another suggestion from Raj:

          In the discussion here: https://moodle.org/mod/forum/discuss.php?d=213649#p932194 there is an alternative where the icons are separated into 3 groups: movement, editing and status - if we add this type of grouping into the actionmenuitem class (proposed) above and put it in the html - then it could be used by themers (even if we do not make use it).

          Show
          Damyon Wiese added a comment - Another suggestion from Raj: In the discussion here: https://moodle.org/mod/forum/discuss.php?d=213649#p932194 there is an alternative where the icons are separated into 3 groups: movement, editing and status - if we add this type of grouping into the actionmenuitem class (proposed) above and put it in the html - then it could be used by themers (even if we do not make use it).
          Hide
          Damyon Wiese added a comment -

          That's probably enough for now - feel free to send for review again later.

          Show
          Damyon Wiese added a comment - That's probably enough for now - feel free to send for review again later.
          Hide
          Martin Dougiamas added a comment -

          I don't think a new issue should have been created for this, Jason, we had this one already with 24 watchers on it: MDL-35985

          Show
          Martin Dougiamas added a comment - I don't think a new issue should have been created for this, Jason, we had this one already with 24 watchers on it: MDL-35985
          Hide
          Jason Fowler added a comment -

          This issue was originally raised with the intention of just spacing the icons out, and making them larger. It came out of the testing for mobile devices. After that, it evolved, with the purpose changing. This issue goes beyond the scope of the other one, to include block icons too.

          Before raising this issue, I did a search for the topic I was raising the issue to address, that is icons being too small, and found no results.

          Show
          Jason Fowler added a comment - This issue was originally raised with the intention of just spacing the icons out, and making them larger. It came out of the testing for mobile devices. After that, it evolved, with the purpose changing. This issue goes beyond the scope of the other one, to include block icons too. Before raising this issue, I did a search for the topic I was raising the issue to address, that is icons being too small, and found no results.
          Hide
          Martin Dougiamas added a comment -

          OK, well I've expanded the scope of this one because I think it all needs to be considered together and properly, and closed the other one.

          Show
          Martin Dougiamas added a comment - OK, well I've expanded the scope of this one because I think it all needs to be considered together and properly, and closed the other one.
          Hide
          Tim Hunt added a comment -

          It is not clear to me whether "All types of screens" means all devices, or being consistent throughout the Moodle UI.

          It looks like you are just considering course page and blocks. Is that right?

          (Other places I am aware of are mod/quiz/edit.php and question/edit.php, but I guess you don't want to work on them right now.)

          Show
          Tim Hunt added a comment - It is not clear to me whether "All types of screens" means all devices, or being consistent throughout the Moodle UI. It looks like you are just considering course page and blocks. Is that right? (Other places I am aware of are mod/quiz/edit.php and question/edit.php, but I guess you don't want to work on them right now.)
          Hide
          Jason Fowler added a comment -

          I think that "All types of screens" means Desktop/Laptop, Tablet and Phone. At the moment, blocks and courses are the only ones getting an over haul, but I think if this becomes the standard, it should be used throughout moodle, including in the quiz and questions.

          Show
          Jason Fowler added a comment - I think that "All types of screens" means Desktop/Laptop, Tablet and Phone. At the moment, blocks and courses are the only ones getting an over haul, but I think if this becomes the standard, it should be used throughout moodle, including in the quiz and questions.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I am putting this up for peer-review again now.

          The following changes have been made:

          1. 77b3a65 Better handling of default display of menu (reduce visual jump)
          2. 77b3a65 Improved display of activities when the menu is visible.
          3. b07bd10 Introduced an action_menu_action class and converted to it.
          4. b07bd10 Tided up display of moveleft when you can't move left.
          5. 0ca83ca Increased the size + padding of the activity editing icons
          6. 94142b7 Redeveloped the panel to fit within page flow - fixing tab order issues.

          Regarding other points:

          • Open the menu's to the right not the left. Funnily enough I feel the same as Damyon, I like them opening as they are so I've left it.
          • Move the move icon to the left of the activity name. Definitely another issue, worth noting is that moving it like that will lead to a more noticable visual jump.
          • I like the moveright + moveleft in the menu myself as well, keeping things outside of the menu minimal is ideal I think.
          • Regarding the idea of splitting into three groups, I don't really get how that would work with this drop down menu. Sorry - leaving this. Another issue if we must.
          • This shouldn't have been a new issue.
          • Tim at the moment course + blocks have been converted to use the new "action_menu" component. We could create new issues to convert quiz/question. I think these changes will be controversial enough and I'd like them in before expanding out.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I am putting this up for peer-review again now. The following changes have been made: 77b3a65 Better handling of default display of menu (reduce visual jump) 77b3a65 Improved display of activities when the menu is visible. b07bd10 Introduced an action_menu_action class and converted to it. b07bd10 Tided up display of moveleft when you can't move left. 0ca83ca Increased the size + padding of the activity editing icons 94142b7 Redeveloped the panel to fit within page flow - fixing tab order issues. Regarding other points: Open the menu's to the right not the left. Funnily enough I feel the same as Damyon, I like them opening as they are so I've left it. Move the move icon to the left of the activity name. Definitely another issue, worth noting is that moving it like that will lead to a more noticable visual jump. I like the moveright + moveleft in the menu myself as well, keeping things outside of the menu minimal is ideal I think. Regarding the idea of splitting into three groups, I don't really get how that would work with this drop down menu. Sorry - leaving this. Another issue if we must. This shouldn't have been a new issue. Tim at the moment course + blocks have been converted to use the new "action_menu" component. We could create new issues to convert quiz/question. I think these changes will be controversial enough and I'd like them in before expanding out. Many thanks Sam
          Hide
          Tim Hunt added a comment -

          I agree it is sensible to do activities and blocks now, and see how that goes before do more changes elsewhere. I am planning some work on the quiz edit page later this calendar year, and so might be able to make this sort of change there when the time comes.

          Show
          Tim Hunt added a comment - I agree it is sensible to do activities and blocks now, and see how that goes before do more changes elsewhere. I am planning some work on the quiz edit page later this calendar year, and so might be able to make this sort of change there when the time comes.
          Hide
          Jason Fowler added a comment -

          Trivial, but you were cleaning up the comments else where, https://github.com/samhemelryk/moodle/compare/master...wip-MDL-39814-m26#L0R1863 needs a capital at the beginning.

          Other than that I didn't see anything wrong with the code. Should be good to go.

          Show
          Jason Fowler added a comment - Trivial, but you were cleaning up the comments else where, https://github.com/samhemelryk/moodle/compare/master...wip-MDL-39814-m26#L0R1863 needs a capital at the beginning. Other than that I didn't see anything wrong with the code. Should be good to go.
          Hide
          Barbara Ramiro added a comment -

          Hi Sam,

          This is what i see... z-index issue.

          Show
          Barbara Ramiro added a comment - Hi Sam, This is what i see... z-index issue.
          Hide
          Barbara Ramiro added a comment - - edited

          Also, when moving to the left/right, the context menu moves to the right edge of the loading animation before it goes to where it should be. Is it possible to put the loading animation outside the span/div or somewhere else?

          This is what i see...
          . . .

          . . .

          Show
          Barbara Ramiro added a comment - - edited Also, when moving to the left/right, the context menu moves to the right edge of the loading animation before it goes to where it should be. Is it possible to put the loading animation outside the span/div or somewhere else? This is what i see... . . . . . .
          Hide
          Sam Hemelryk added a comment -

          Thanks for picking those up Barbara.
          I've made a fix for them now and I'm putting this up for integration review.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for picking those up Barbara. I've made a fix for them now and I'm putting this up for integration review. Many thanks Sam
          Hide
          Marina Glancy added a comment -

          Hi Sam, this change looks very good.

          A couple of things I've noticed in the code:

          • this needs mentioning in theme/upgrade.txt. First, themes may not be based on base or bootstrapbase. Second, theme may have overwritten core_course_renderer::course_section_cm_edit_actions()
          • also in course/format/upgrade.txt (for formats that do not use core_course_renderer::course_section_cm_edit_actions() for example )
          • is it possible for course formats to continue displaying icons in one line if they want or group icons differently? It looks like course_get_cm_edit_actions() hardcodes the level of each icon and it makes it a little inflexible. And this is library function, it is not in renderer and can't be overridden. I don't like very much the changes in course_get_cm_edit_actions(), IMHO renderer should convert icons from action_link to action_menu_action.

          Testing instructions are missing, hope they cover also: non-javascript mode, AJAX-disabled mode and mobile device with touch screen.

          Did you run behat tests? The UI changes are quite significant, hope it does not break the tests.

          Show
          Marina Glancy added a comment - Hi Sam, this change looks very good. A couple of things I've noticed in the code: this needs mentioning in theme/upgrade.txt. First, themes may not be based on base or bootstrapbase. Second, theme may have overwritten core_course_renderer::course_section_cm_edit_actions() also in course/format/upgrade.txt (for formats that do not use core_course_renderer::course_section_cm_edit_actions() for example ) is it possible for course formats to continue displaying icons in one line if they want or group icons differently? It looks like course_get_cm_edit_actions() hardcodes the level of each icon and it makes it a little inflexible. And this is library function, it is not in renderer and can't be overridden. I don't like very much the changes in course_get_cm_edit_actions(), IMHO renderer should convert icons from action_link to action_menu_action. Testing instructions are missing, hope they cover also: non-javascript mode, AJAX-disabled mode and mobile device with touch screen. Did you run behat tests? The UI changes are quite significant, hope it does not break the tests.
          Hide
          Sam Hemelryk added a comment -

          Hi Marina,

          Addressing your points:

          1. Done
          2. Done
          3. It looks to me as though sections could still take control as they can now by altering how they render sections and producing the HTML they desire rather than calling core_course_renderer::course_section_cm_list as they do now.
          4. (really 3.2) I confess that the current use of the action_menu_actions's isn't really to my liking either. Its an awkward mix of display and logic. I think the ideal solution would be to have course_get_cm_edit_actions produce "module_action" objects rather than output components and then have the render do what it wants with them. It'd be cool if the module_action object had get_action_link() and get_action_menu_action() methods to boot. For this change I had hoped that changing from one output component to another would be passable and both were renderable. It would only throw off overridden renderers that expect an output component. Perhaps a compromise would be to create a new issue to move to the module_action object after this lands and clean up that area properly if you agree with my thoughts.
          5. Fixed up the testing instructions (whoops put a comment there instead of the testing instructions).
          6. Running behat tests right now - I'll let you know how it goes.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Marina, Addressing your points: 1. Done 2. Done 3. It looks to me as though sections could still take control as they can now by altering how they render sections and producing the HTML they desire rather than calling core_course_renderer::course_section_cm_list as they do now. 4. (really 3.2) I confess that the current use of the action_menu_actions's isn't really to my liking either. Its an awkward mix of display and logic. I think the ideal solution would be to have course_get_cm_edit_actions produce "module_action" objects rather than output components and then have the render do what it wants with them. It'd be cool if the module_action object had get_action_link() and get_action_menu_action() methods to boot. For this change I had hoped that changing from one output component to another would be passable and both were renderable. It would only throw off overridden renderers that expect an output component. Perhaps a compromise would be to create a new issue to move to the module_action object after this lands and clean up that area properly if you agree with my thoughts. 5. Fixed up the testing instructions (whoops put a comment there instead of the testing instructions). 6. Running behat tests right now - I'll let you know how it goes. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Aha there are indeed behat test failure.
          Working on those now -

          Show
          Sam Hemelryk added a comment - Aha there are indeed behat test failure. Working on those now -
          Hide
          Martin Dougiamas added a comment -

          -1 for this until we have a user preference to turn it on/off, because I'm worried about accessibility of it right now.

          So
          a) Add the option, which will allow it to be integrated without disadvantaging anyone
          b) Make sure the menu part is very accessible

          Show
          Martin Dougiamas added a comment - -1 for this until we have a user preference to turn it on/off, because I'm worried about accessibility of it right now. So a) Add the option, which will allow it to be integrated without disadvantaging anyone b) Make sure the menu part is very accessible
          Hide
          Marina Glancy added a comment -

          Ok, reopening then.

          Show
          Marina Glancy added a comment - Ok, reopening then.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Damyon Wiese added a comment -

          Hi Sam - just looking at the latest branch.

          I like the bigger icons and the new renderable classes.

          Some small UI things I spotted:

          When an activity is all the way left and the "Move left" menu item is hidden - it still takes up vertical space in the menu (so there is a gap).

          The icons for the block actionmenus are still small

          The menu items in the block menus are right aligned (looks odd).

          Show
          Damyon Wiese added a comment - Hi Sam - just looking at the latest branch. I like the bigger icons and the new renderable classes. Some small UI things I spotted: When an activity is all the way left and the "Move left" menu item is hidden - it still takes up vertical space in the menu (so there is a gap). The icons for the block actionmenus are still small The menu items in the block menus are right aligned (looks odd).
          Hide
          Damyon Wiese added a comment -

          And reposting these two chat comments so they don't get lost:

          Tyops: typo in the function name: "do_not_enahnce"
          one more typo in theme.base/style/course.css "{ertical-align

          Show
          Damyon Wiese added a comment - And reposting these two chat comments so they don't get lost: Tyops: typo in the function name: "do_not_enahnce" one more typo in theme.base/style/course.css "{ertical-align
          Hide
          Sam Hemelryk added a comment -

          Aha thanks Damyon.

          To reply to your points:

          1. move left takes up space - I'll have a look at that. I have changed the way we dealt with that because for this it was an inaccessible mess and then after changing I had issues because of shitty non-descriptive selectors and in fixing that I must've caused the empty space issue and not spotted it.
          2. Small icons for blocks - Yip, I've left them intentionally small. We'll have to create a myriad of icon issues after this (probably an epic here) as there are some gotcha's and missing large icons there.
          3. Yip - I had done that intentionally but if you think best I will revert.
          4. Will most certainly fix the two typos!

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Aha thanks Damyon. To reply to your points: move left takes up space - I'll have a look at that. I have changed the way we dealt with that because for this it was an inaccessible mess and then after changing I had issues because of shitty non-descriptive selectors and in fixing that I must've caused the empty space issue and not spotted it. Small icons for blocks - Yip, I've left them intentionally small. We'll have to create a myriad of icon issues after this (probably an epic here) as there are some gotcha's and missing large icons there. Yip - I had done that intentionally but if you think best I will revert. Will most certainly fix the two typos! Cheers Sam
          Hide
          Damyon Wiese added a comment -

          Yep the menus look odd to me right aligned - especially when they wrap - I was testing on a window about 900px wide. In particular I think the icons should line up.

          Show
          Damyon Wiese added a comment - Yep the menus look odd to me right aligned - especially when they wrap - I was testing on a window about 900px wide. In particular I think the icons should line up.
          Hide
          Sam Hemelryk added a comment -

          Ok all updated, behat tests passing, this is ready once more.

          Show
          Sam Hemelryk added a comment - Ok all updated, behat tests passing, this is ready once more.
          Hide
          Sam Hemelryk added a comment -

          I am going to request a peer-review. Really I just need as many eyes looking at this as possible.

          Show
          Sam Hemelryk added a comment - I am going to request a peer-review. Really I just need as many eyes looking at this as possible.
          Hide
          Andrew Davis added a comment - - edited

          Hi. Consider squishing some/all of those commits. I feel like the commit list shouldn't require me to scroll to see it all

          https://tracker.moodle.org/secure/attachment/33567/gap.png
          Is the gap after "edit title" deliberate?

          Perhaps "Update" should say "Settings".

          Not sure if this falls within this issue but the 3 images for activity group mode are very difficult to tell apart. I actually clicked it a few times wondering what was going on before I even noticed that the icon was changing. On my desktop I was able to consult the tooltip for information. On the phone you're left to guess. Perhaps the group icon should also be a drop down. Click it, get menu of group options, select one. If I get a menu saying no groups, separate groups and visible groups Im at least clear on what exactly I'm selecting.

          Does the group icon need to be a top level icon? It seems odd that groups is right there but the activity settings are tucked away in the menu.

          The Settings/Update icon is outside the drop down menu for blocks but within the menu for activities. They should probably be the same. My off the cuff suggestion is that the activity be edited to move groups within the menu while Settings/Update should be on the page.

          I think you might be missing two var statements. In change_visibility() and a few other places there is "var spinner = " creating a new local variable called spinner. However in change_groupmode() and change_indent() the is just "spinner = " creating a new global variable if Im not mistaken.

          Actually, the code is correct. Its just written in a somewhat unclear way.

                      indentdiv.addClass(CSS.MODINDENTCOUNT + newindent);
                      var data = {
                          'class' : 'resource',
                          'field' : 'indent',
                          'value' : newindent,
                          'id' : this.get_element_id(activity)
                          },
                          commands = activity.one(SELECTOR.COMMANDSPAN),
                          spinner = M.util.add_spinner(Y, commands).setStyles({
                              position: 'absolute',
                              top: 0
                          });
                      if (BODY.hasClass('dir-ltr')) {
                         ...
          

          I'd suggest that we not use comma separated var declarations if it contains any object definitions. Using several vars performs the same, is easier to read, is less prone to weird scope bugs (http://stackoverflow.com/questions/694102/declaring-multiple-variables-in-javascript/7384541#7384541) and avoids the non-matching indent of the original.

          var data = {
             ...
          }
          var commands = ...
          var spinner = M.util.add_spinner(Y, commands).setStyles({
              ...
          });
          

          This I don't mind as it at least doesn't contain any object definitions.

          +            var groupmode = parseInt(button.getData('nextgroupmode'), 10),
          +                newtitle = '',
          +                iconsrc = '',
          +                newtitlestr,
          +                data,
          +                spinner,
          +                nextgroupmode = groupmode + 1; 
          
          Show
          Andrew Davis added a comment - - edited Hi. Consider squishing some/all of those commits. I feel like the commit list shouldn't require me to scroll to see it all https://tracker.moodle.org/secure/attachment/33567/gap.png Is the gap after "edit title" deliberate? Perhaps "Update" should say "Settings". Not sure if this falls within this issue but the 3 images for activity group mode are very difficult to tell apart. I actually clicked it a few times wondering what was going on before I even noticed that the icon was changing. On my desktop I was able to consult the tooltip for information. On the phone you're left to guess. Perhaps the group icon should also be a drop down. Click it, get menu of group options, select one. If I get a menu saying no groups, separate groups and visible groups Im at least clear on what exactly I'm selecting. Does the group icon need to be a top level icon? It seems odd that groups is right there but the activity settings are tucked away in the menu. The Settings/Update icon is outside the drop down menu for blocks but within the menu for activities. They should probably be the same. My off the cuff suggestion is that the activity be edited to move groups within the menu while Settings/Update should be on the page. I think you might be missing two var statements. In change_visibility() and a few other places there is "var spinner = " creating a new local variable called spinner. However in change_groupmode() and change_indent() the is just "spinner = " creating a new global variable if Im not mistaken. Actually, the code is correct. Its just written in a somewhat unclear way. indentdiv.addClass(CSS.MODINDENTCOUNT + newindent); var data = { 'class' : 'resource', 'field' : 'indent', 'value' : newindent, 'id' : this .get_element_id(activity) }, commands = activity.one(SELECTOR.COMMANDSPAN), spinner = M.util.add_spinner(Y, commands).setStyles({ position: 'absolute', top: 0 }); if (BODY.hasClass('dir-ltr')) { ... I'd suggest that we not use comma separated var declarations if it contains any object definitions. Using several vars performs the same, is easier to read, is less prone to weird scope bugs ( http://stackoverflow.com/questions/694102/declaring-multiple-variables-in-javascript/7384541#7384541 ) and avoids the non-matching indent of the original. var data = { ... } var commands = ... var spinner = M.util.add_spinner(Y, commands).setStyles({ ... }); This I don't mind as it at least doesn't contain any object definitions. + var groupmode = parseInt(button.getData('nextgroupmode'), 10), + newtitle = '', + iconsrc = '', + newtitlestr, + data, + spinner, + nextgroupmode = groupmode + 1;
          Hide
          Andrew Davis added a comment -

          Minor point. Quite a few comments are missing the full stop. (I'm assuming the JS should adhere to the same guidelines as the PHP)

          // Detach all listen events to prevent duplicate triggers

          // Section Highlighting

          // Section Highlighting

          There's more. Either fix them in this issue or open a new MDL.

          Show
          Andrew Davis added a comment - Minor point. Quite a few comments are missing the full stop. (I'm assuming the JS should adhere to the same guidelines as the PHP) // Detach all listen events to prevent duplicate triggers // Section Highlighting // Section Highlighting There's more. Either fix them in this issue or open a new MDL.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Thanks for looking that over.
          In reply to your points:

          1. The commits: I'll interactivily rebase for putting this up for integration review and shrink the peer-review touch up commits into their parents as they're not overly meaningful. That'll drop it back to around 10 meaningful commits which I feel is pretty fair for a patch like this that works across several areas.
          2. The gap after edit-title is a bug. I've fixed that now. It appeared during the accessibility changes where I changed the nested but missed updating a selector to reflect the structure change.
          3. Those activity icons should be turned into a separate issue I feel. That design has been used for a long time, however I think the change to monochrome has probably weaken their standing as from memory the old icons used to use different colours for the people making it much more obvious. I'm not really sure hwo they could be clarified but perhaps Barbara would have some ideas.
          4. Yeah I am unsure about the group icons belonging there as well, however as I understand it that came from either reseacrh done by Raj/Damyon or from forum suggestions. I think for the purposes of this patch we should leave it there and then see what people think after its gets into use. Its a very easy change to move it and I don't think this issue should be delayed further pending more research into that.
          5. We've been using update there for quite some time. I know that it was changed from settings to update although for the life or me I can't remember why. Interestingly enough it is still referred to as settings in the settings block. I'll open an issue for this and we can decide once and for all whether it should be update or settings and then update our code as required.
          6. The notion of multi-var vs. single var coding style is an interesting one. A single var declaration at the top of the block has unofficially been the JS communities preferred approach for many years. There are really two parts to that coding style, the first being that all variables are defined at the top which makes good sense as JS implements variable hoisting and when interpretted this ensures the writted code is closer to the interpreted result. However as far as I am aware the single var approach was adopted more for readability and because of its visual appearance. Noting that both single and multi var declarations function correctly in strict mode.
            My personal preference is towards single var statements, perhaps more because it is what I am used to writing more than anything. On the more practical side tools such as jshint and jslint by default check for and report warnings if multi var statements are used. It also used to be that multiple var statements led to higher character counts however as we run our JS throught a minifcation process that is not an issue. Although I suppose in that case the same reasoning could be applied to this as has been applied to declarations being made at the top. Minifiers will reduce the end code to a single var declaration, in trying to maintain a closeness to the code we will deliver we should also use a single var declaration.
            As we've not really got good coding style guidelines for JS yet this issue could still go either way, what I think is important is consistency. In this case I have split the suspect code into several var statements as you've suggested because it is consistent with the rest of the code in that function.
            What I purpose is that we create a policy issue for this and get the likes of Andrew N in for his thoughts before finialising such a policy. For sure my vote goes to a single var declaration.
          7. Ah yes the fullstops. I've added fullstops to the comments I've added or edited where I had missed them. However I won't fix any comments on lines I've not modified as I think Andrew N may be working on this file as well and I'd hate to cause him more conflict that needed if this lands first.

          Putting this up for integration review now.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Thanks for looking that over. In reply to your points: The commits: I'll interactivily rebase for putting this up for integration review and shrink the peer-review touch up commits into their parents as they're not overly meaningful. That'll drop it back to around 10 meaningful commits which I feel is pretty fair for a patch like this that works across several areas. The gap after edit-title is a bug. I've fixed that now. It appeared during the accessibility changes where I changed the nested but missed updating a selector to reflect the structure change. Those activity icons should be turned into a separate issue I feel. That design has been used for a long time, however I think the change to monochrome has probably weaken their standing as from memory the old icons used to use different colours for the people making it much more obvious. I'm not really sure hwo they could be clarified but perhaps Barbara would have some ideas. Yeah I am unsure about the group icons belonging there as well, however as I understand it that came from either reseacrh done by Raj/Damyon or from forum suggestions. I think for the purposes of this patch we should leave it there and then see what people think after its gets into use. Its a very easy change to move it and I don't think this issue should be delayed further pending more research into that. We've been using update there for quite some time. I know that it was changed from settings to update although for the life or me I can't remember why. Interestingly enough it is still referred to as settings in the settings block. I'll open an issue for this and we can decide once and for all whether it should be update or settings and then update our code as required. The notion of multi-var vs. single var coding style is an interesting one. A single var declaration at the top of the block has unofficially been the JS communities preferred approach for many years. There are really two parts to that coding style, the first being that all variables are defined at the top which makes good sense as JS implements variable hoisting and when interpretted this ensures the writted code is closer to the interpreted result. However as far as I am aware the single var approach was adopted more for readability and because of its visual appearance. Noting that both single and multi var declarations function correctly in strict mode. My personal preference is towards single var statements, perhaps more because it is what I am used to writing more than anything. On the more practical side tools such as jshint and jslint by default check for and report warnings if multi var statements are used. It also used to be that multiple var statements led to higher character counts however as we run our JS throught a minifcation process that is not an issue. Although I suppose in that case the same reasoning could be applied to this as has been applied to declarations being made at the top. Minifiers will reduce the end code to a single var declaration, in trying to maintain a closeness to the code we will deliver we should also use a single var declaration. As we've not really got good coding style guidelines for JS yet this issue could still go either way, what I think is important is consistency. In this case I have split the suspect code into several var statements as you've suggested because it is consistent with the rest of the code in that function. What I purpose is that we create a policy issue for this and get the likes of Andrew N in for his thoughts before finialising such a policy. For sure my vote goes to a single var declaration. Ah yes the fullstops. I've added fullstops to the comments I've added or edited where I had missed them. However I won't fix any comments on lines I've not modified as I think Andrew N may be working on this file as well and I'd hate to cause him more conflict that needed if this lands first. Putting this up for integration review now. Many thanks Sam
          Hide
          Tim Hunt added a comment -

          Re 5: I am wondering if it is a noun/verb thing. If you ask what the settings are, well they are settings, and saying what they are is consistent with the rest of the admin block (Groups, Reports, Grades, ...). On the other hand, buttons/icons are more often verbs to do actions, in this case update. To make it clearer, you could change it to update settings. (I guess it really ought to be update settings - a verb phrase, not just a verb.)

          Show
          Tim Hunt added a comment - Re 5: I am wondering if it is a noun/verb thing. If you ask what the settings are, well they are settings, and saying what they are is consistent with the rest of the admin block (Groups, Reports, Grades, ...). On the other hand, buttons/icons are more often verbs to do actions, in this case update. To make it clearer, you could change it to update settings. (I guess it really ought to be update settings - a verb phrase, not just a verb.)
          Hide
          Dan Poltawski added a comment - - edited

          Hi Sam,

          In chrome - there appears to be a sizing issue when clicking on the group/hide icons changing when clicked.

          See the following screenshots:

          Show
          Dan Poltawski added a comment - - edited Hi Sam, In chrome - there appears to be a sizing issue when clicking on the group/hide icons changing when clicked. See the following screenshots:
          Hide
          Dan Poltawski added a comment - - edited

          Also, site_main_menu block is broken:

          You should upgrade your call to course_section_cm_edit_actions and provide $mod
          line 348 of /course/renderer.php: call to debugging()
          line 93 of /blocks/site_main_menu/block_site_main_menu.php: call to core_course_renderer->course_section_cm_edit_actions()
          line 772 of /blocks/moodleblock.class.php: call to block_site_main_menu->get_content()
          line 238 of /blocks/moodleblock.class.php: call to block_list->formatted_contents()
          line 956 of /lib/blocklib.php: call to block_base->get_content_for_output()
          line 1008 of /lib/blocklib.php: call to block_manager->create_block_contents()
          line 353 of /lib/blocklib.php: call to block_manager->ensure_content_created()
          line 3193 of /lib/outputrenderers.php: call to block_manager->region_has_content()
          line 3229 of /lib/outputrenderers.php: call to core_renderer->body_css_classes()
          line 49 of /theme/clean/layout/columns3.php: call to core_renderer->body_attributes()
          line 847 of /lib/outputrenderers.php: call to include()
          line 777 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
          line 101 of /index.php: call to core_renderer->header()
          
          Show
          Dan Poltawski added a comment - - edited Also, site_main_menu block is broken: You should upgrade your call to course_section_cm_edit_actions and provide $mod line 348 of /course/renderer.php: call to debugging() line 93 of /blocks/site_main_menu/block_site_main_menu.php: call to core_course_renderer->course_section_cm_edit_actions() line 772 of /blocks/moodleblock.class.php: call to block_site_main_menu->get_content() line 238 of /blocks/moodleblock.class.php: call to block_list->formatted_contents() line 956 of /lib/blocklib.php: call to block_base->get_content_for_output() line 1008 of /lib/blocklib.php: call to block_manager->create_block_contents() line 353 of /lib/blocklib.php: call to block_manager->ensure_content_created() line 3193 of /lib/outputrenderers.php: call to block_manager->region_has_content() line 3229 of /lib/outputrenderers.php: call to core_renderer->body_css_classes() line 49 of /theme/clean/layout/columns3.php: call to core_renderer->body_attributes() line 847 of /lib/outputrenderers.php: call to include() line 777 of /lib/outputrenderers.php: call to core_renderer->render_page_layout() line 101 of /index.php: call to core_renderer->header()
          Hide
          David Monllaó added a comment - - edited

          Ohh I like this changes; comments about the feature files:

          • Remove @sam tag from course/tests/behat/course_controls.feature
          • In course/tests/behat/activities_indentation.feature instead of not checking the existence of the 'Move left' option we could replace the xpath to another one adapted to the new DOM, could be //*[@id='section-1']/descendant::li[contains(concat(' ', @class, ' '), ' glossary ')]/descendant::span[normalize-space(.)='Move left'] considering that now the extra actions are span texts rather than images.
          • The steps in course/tests/behat/behat_course.php are compatible with JS enabled and JS disabled; with JS disabled there is no 'Actions' links and all the icons are displayed as usual, so in methods like i_indent_right_activity() the click to actions should only be done if $this->running_javascript()

          It smells like it will conflict with MDL-39635 changes that will also be integrated this week, I talked with Dan and I'm rebasing MDL-39635 on top of this issue and solve the conflicts there.

          Show
          David Monllaó added a comment - - edited Ohh I like this changes; comments about the feature files: Remove @sam tag from course/tests/behat/course_controls.feature In course/tests/behat/activities_indentation.feature instead of not checking the existence of the 'Move left' option we could replace the xpath to another one adapted to the new DOM, could be //* [@id='section-1'] /descendant::li [contains(concat(' ', @class, ' '), ' glossary ')] /descendant::span [normalize-space(.)='Move left'] considering that now the extra actions are span texts rather than images. The steps in course/tests/behat/behat_course.php are compatible with JS enabled and JS disabled; with JS disabled there is no 'Actions' links and all the icons are displayed as usual, so in methods like i_indent_right_activity() the click to actions should only be done if $this->running_javascript() It smells like it will conflict with MDL-39635 changes that will also be integrated this week, I talked with Dan and I'm rebasing MDL-39635 on top of this issue and solve the conflicts there.
          Hide
          David Monllaó added a comment -

          Hi Sam, I've created a new MDL-39635 branch rebasing this issue's branch to solve conflicts, I've also applied the proposed behat-related files changes to avoid more conflicts when integrating MDL-39635 if you change them here; let me know if you are not happy with it or do you prefer to do it in another way.

          https://tracker.moodle.org/browse/MDL-39635?focusedCommentId=234529&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-234529

          Show
          David Monllaó added a comment - Hi Sam, I've created a new MDL-39635 branch rebasing this issue's branch to solve conflicts, I've also applied the proposed behat-related files changes to avoid more conflicts when integrating MDL-39635 if you change them here; let me know if you are not happy with it or do you prefer to do it in another way. https://tracker.moodle.org/browse/MDL-39635?focusedCommentId=234529&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-234529
          Hide
          Andrew Nicols added a comment -

          I've just been having a look through this issue very briefly. Just wanted to check that someone has tested the JS on an older machine, running an older browser (e.g. IE8), on a course with lots of activities.

          We make a lot of DOM changes on load and it would just be reassuring to make sure!

          Show
          Andrew Nicols added a comment - I've just been having a look through this issue very briefly. Just wanted to check that someone has tested the JS on an older machine, running an older browser (e.g. IE8), on a course with lots of activities. We make a lot of DOM changes on load and it would just be reassuring to make sure!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm reopening this (cool) issue now, it seems that there are some points to verify/fix before allowing this to land and Sam is not around this week so:

          1) site_main_menu block breakage.
          2) some fixes to behat tests (or alternatively also hold MDL-39635 and integrate it after this, that's under discussion right now).
          3) concerns with IE8?
          4) codechecker revealing some easy to fix issues: http://ci.stronk7.com/job/Precheck%20remote%20branch/359/artifact/work/smurf.html

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm reopening this (cool) issue now, it seems that there are some points to verify/fix before allowing this to land and Sam is not around this week so: 1) site_main_menu block breakage. 2) some fixes to behat tests (or alternatively also hold MDL-39635 and integrate it after this, that's under discussion right now). 3) concerns with IE8? 4) codechecker revealing some easy to fix issues: http://ci.stronk7.com/job/Precheck%20remote%20branch/359/artifact/work/smurf.html Ciao
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Sam Hemelryk added a comment -

          Aha thanks guys, I'll look at those points over the next week.
          Regarding MDL-39635 I am happy either way, if it lands now no probs I'll resolve the conflicts I have on my end, otherwise if we choose to wait thats cool too.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Aha thanks guys, I'll look at those points over the next week. Regarding MDL-39635 I am happy either way, if it lands now no probs I'll resolve the conflicts I have on my end, otherwise if we choose to wait thats cool too. Many thanks Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks Sam, after talking about it with David we are going to try integrating those (MDL-39635 and MDL-40123) this week, so it's possible you'll find some conflict... although David already had a branch on top of yours, perhaps that will help:

          git://github.com/dmonllao/moodle.git MDL-39635_master-ontopof-mdl-39814

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks Sam, after talking about it with David we are going to try integrating those ( MDL-39635 and MDL-40123 ) this week, so it's possible you'll find some conflict... although David already had a branch on top of yours, perhaps that will help: git://github.com/dmonllao/moodle.git MDL-39635 _master-ontopof-mdl-39814 Ciao
          Hide
          David Scotson added a comment -

          Now that the icons are being displayed alongside explanatory text, the alt text is repetitive and should be removed for accessibility reasons.

          If you're looking for a way to make the icons highlight on hover, because currently it looks a bit odd to have the mid-gray on dark blue, then I'd suggest MDL-40759. If you made the above accessibility change, then it's possible that it might "just work" already in the Essential theme as it replaces decorative icons with a font via a renderer, but currently only those used decoratively.

          Show
          David Scotson added a comment - Now that the icons are being displayed alongside explanatory text, the alt text is repetitive and should be removed for accessibility reasons. If you're looking for a way to make the icons highlight on hover, because currently it looks a bit odd to have the mid-gray on dark blue, then I'd suggest MDL-40759 . If you made the above accessibility change, then it's possible that it might "just work" already in the Essential theme as it replaces decorative icons with a font via a renderer, but currently only those used decoratively.
          Hide
          Sam Hemelryk added a comment -

          Thanks for the help guys, I'm putting this back up for integration with the following having now been done:

          • Resolved Behat conflicts as per David's patch resolving the conflicts last week.
          • Fixed up site_main_menu and two other places using that function so they don't throw debugging notices.
          • Corrected most (not all) codechecker notices.
          • Fixed a bug with IE8 display of icons caused by bootstrap reset wierdness.
          • Removed alt attibutes from icons that now have descriptive text. (thanks David for picking that one up you were dead right).
          • Tweaked actionmenu initialisation for performance.

          I've also run IE8 profiling across these changes. To summarise on a course with 20+ blocks and 30+ modules:
          Combined JS execution time:

          Without patch With patch applied
          27718.98 27189.05

          That's an increase of just 0.25%.

          Upon my initial run I had an increase of 12% however after minor tweaking I was able to reduce that to 0.25% pretty easily.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for the help guys, I'm putting this back up for integration with the following having now been done: Resolved Behat conflicts as per David's patch resolving the conflicts last week. Fixed up site_main_menu and two other places using that function so they don't throw debugging notices. Corrected most (not all) codechecker notices. Fixed a bug with IE8 display of icons caused by bootstrap reset wierdness. Removed alt attibutes from icons that now have descriptive text. (thanks David for picking that one up you were dead right). Tweaked actionmenu initialisation for performance. I've also run IE8 profiling across these changes. To summarise on a course with 20+ blocks and 30+ modules: Combined JS execution time: Without patch With patch applied 27718.98 27189.05 That's an increase of just 0.25%. Upon my initial run I had an increase of 12% however after minor tweaking I was able to reduce that to 0.25% pretty easily. Many thanks Sam
          Hide
          Frédéric Massart added a comment -

          Hi everyone,

          allow me to add my 2 cents here. I am conscious that this issue has already been discussed a lot, but nonetheless I would like to express some thoughts.

          I have troubles with the icons which are not in the dropdown. I understand why the "Move" icon isn't, that makes sense. However, the group icon does not. I have been told that it was there to display the current status of the group setting, which helps teachers to quickly see what mode they've set on the activity. Though I don't think it's a valid reason to have the icon out of the dropdown. If ever we want to display the group status, then we could display an icon, or some text (as we do for grouping). The "Hide/Show" icon is not important enough to me to be out of the dropdown either. If there is a dropdown, and an icon is accessible from outside, by definition it means that it is so important that it should be accessible very quickly, be used very often. Is that the case? I doubt it. The dimmed class would notify the user if the activity is hidden or not anyway.

          In regard with the positioning of the dropdown, have we tried to place it in between the activity icon and the activity title? That way, the dropdown would not end up being placed at different places for each activity. And the "Move" icon could be replaced by a "marker" (sometimes 3 vertical dots) on which the cursor becomes the typical "move". This would be neat and clean. Though I assume that as this doesn't work on touch screens, we would need an alternate solution anyway.

          About the "Edit title", I don't think this one should be in the dropdown, but next to the activity name. An "Edit-in-place" action should not be part of it, that makes little sense.

          About the dropdown icon, I think it's very important to pick the right one and stick with it because we might end up using it all over the place. And I have to say that I'm not loving that one. Was that a problem to use a common (i/settings for instance) icon along with the little arrow down that Bootstrap uses? And if the dropdown has a bit too many items, it might be a bit confusing for some users, would it be possible group some and separate them with a "divider"?

          I also overlooked the actions on the other themes, and there seem to be some wrong alignments, or background colouring. And in Clean the layout of this dropdown seems to differ from the Bootstrap one (custom menu etc...). I also agree with Andrew about "Update" which should probably be "Settings", it is "Settings" in the settings block anyway.

          I am not sure I understood why we added a dropdown for 2 icons (as far as I can tell) for the block editing icons. What is the value of adding a click to display 2 options only?

          Anyway, I can tell that there has been some hard work here, and far from me is the idea to neglect that fact! Last week I was playing with some Frontend stuff (https://docs.google.com/document/d/16BJsINun13StUFOQaejj3Gawz9jk22ir_r49f3MbInM/edit#) and as it's fresh in my mind I thought that I could share some thoughts.

          Thanks guys!
          Fred

          PS: I already plead guilty for not having read the whole thread...

          Show
          Frédéric Massart added a comment - Hi everyone, allow me to add my 2 cents here. I am conscious that this issue has already been discussed a lot, but nonetheless I would like to express some thoughts. I have troubles with the icons which are not in the dropdown. I understand why the "Move" icon isn't, that makes sense. However, the group icon does not. I have been told that it was there to display the current status of the group setting, which helps teachers to quickly see what mode they've set on the activity. Though I don't think it's a valid reason to have the icon out of the dropdown. If ever we want to display the group status, then we could display an icon, or some text (as we do for grouping). The "Hide/Show" icon is not important enough to me to be out of the dropdown either. If there is a dropdown, and an icon is accessible from outside, by definition it means that it is so important that it should be accessible very quickly, be used very often. Is that the case? I doubt it. The dimmed class would notify the user if the activity is hidden or not anyway. In regard with the positioning of the dropdown, have we tried to place it in between the activity icon and the activity title? That way, the dropdown would not end up being placed at different places for each activity. And the "Move" icon could be replaced by a "marker" (sometimes 3 vertical dots) on which the cursor becomes the typical "move". This would be neat and clean. Though I assume that as this doesn't work on touch screens, we would need an alternate solution anyway. About the "Edit title", I don't think this one should be in the dropdown, but next to the activity name. An "Edit-in-place" action should not be part of it, that makes little sense. About the dropdown icon, I think it's very important to pick the right one and stick with it because we might end up using it all over the place. And I have to say that I'm not loving that one. Was that a problem to use a common (i/settings for instance) icon along with the little arrow down that Bootstrap uses? And if the dropdown has a bit too many items, it might be a bit confusing for some users, would it be possible group some and separate them with a "divider"? I also overlooked the actions on the other themes, and there seem to be some wrong alignments, or background colouring. And in Clean the layout of this dropdown seems to differ from the Bootstrap one (custom menu etc...). I also agree with Andrew about "Update" which should probably be "Settings", it is "Settings" in the settings block anyway. I am not sure I understood why we added a dropdown for 2 icons (as far as I can tell) for the block editing icons. What is the value of adding a click to display 2 options only? Anyway, I can tell that there has been some hard work here, and far from me is the idea to neglect that fact! Last week I was playing with some Frontend stuff ( https://docs.google.com/document/d/16BJsINun13StUFOQaejj3Gawz9jk22ir_r49f3MbInM/edit# ) and as it's fresh in my mind I thought that I could share some thoughts. Thanks guys! Fred PS: I already plead guilty for not having read the whole thread...
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          I'm in the frontend sprint presently so I will keep my reply ultra brief:

          1. Which icons are in there. I agree in part - I'm not sure whether we've nailed it yet. As the change would be trivial but the decision large, and as I think this is ready now anyway I would vote we open a new issue to discuss options. As this is master only I think that would be the best way through.
          2. -1 on an accessibility basis from me sorry. Hitting actions before hitting the activity name I think would confuse people. Perhaps a separate issue again to fix alignment. I say separate issue because I know there have been issues in the past for that and it'd require someone to research history perhaps.
          3. Fits with point 1: a new issue to review whats in the drop down and whats not.
          4. Yip I agree the dropdown icon doesn't appeal to me either however that is what was decided upon so I've stuck with it. In regards to using settings I am against that I don't feel we should re-us icons like that especially as that icon is used if the user disabled the action-menu anyway. I think a separate icon is required although I don't know what that separate icon should look like. What we've got works so again...
          5. Update - Settings will need to be a separate issue.
          6. Blocks actions were converted for consistency sake - no other reason really.
          7. Hehe havn't read your doc sorry dude - I'm out of time.

          Thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, I'm in the frontend sprint presently so I will keep my reply ultra brief: Which icons are in there. I agree in part - I'm not sure whether we've nailed it yet. As the change would be trivial but the decision large, and as I think this is ready now anyway I would vote we open a new issue to discuss options. As this is master only I think that would be the best way through. -1 on an accessibility basis from me sorry. Hitting actions before hitting the activity name I think would confuse people. Perhaps a separate issue again to fix alignment. I say separate issue because I know there have been issues in the past for that and it'd require someone to research history perhaps. Fits with point 1: a new issue to review whats in the drop down and whats not. Yip I agree the dropdown icon doesn't appeal to me either however that is what was decided upon so I've stuck with it. In regards to using settings I am against that I don't feel we should re-us icons like that especially as that icon is used if the user disabled the action-menu anyway. I think a separate icon is required although I don't know what that separate icon should look like. What we've got works so again... Update - Settings will need to be a separate issue. Blocks actions were converted for consistency sake - no other reason really. Hehe havn't read your doc sorry dude - I'm out of time. Thanks Sam
          Hide
          David Scotson added a comment -

          Regarding #4, what about the word "edit" with a downward pointing "I'm a drop-down menu" triangle/arrow instead of the icon to trigger the menu?

          Show
          David Scotson added a comment - Regarding #4, what about the word "edit" with a downward pointing "I'm a drop-down menu" triangle/arrow instead of the icon to trigger the menu?
          Hide
          David Scotson added a comment -

          Frédéric Massart, is there an issue tracker or forum discussion for your document? I also thing the navigation block should mostly be migrated into the header bar, and there's been various other people talking about rewriting/changing/improving it so it would be good to get them all together.

          Show
          David Scotson added a comment - Frédéric Massart , is there an issue tracker or forum discussion for your document? I also thing the navigation block should mostly be migrated into the header bar, and there's been various other people talking about rewriting/changing/improving it so it would be good to get them all together.
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          Not reviewed again yet, but looks like this is conflicting with MDL-40678..

          Show
          Dan Poltawski added a comment - Hi Sam, Not reviewed again yet, but looks like this is conflicting with MDL-40678 ..
          Hide
          Frédéric Massart added a comment -

          (David Scotson, no there is no forum discussion or tracker issue for it just yet, but I am planning on creating one.)

          Show
          Frédéric Massart added a comment - ( David Scotson , no there is no forum discussion or tracker issue for it just yet, but I am planning on creating one.)
          Hide
          Sam Hemelryk added a comment -

          Aha right - thanks Dan I integrated MDL-40678 earlier today I should have thought of that.
          Easy conflicts - I've produced a branch based upon integration/master now having resolved the conflicts.

          All yours again

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Aha right - thanks Dan I integrated MDL-40678 earlier today I should have thought of that. Easy conflicts - I've produced a branch based upon integration/master now having resolved the conflicts. All yours again Many thanks Sam
          Hide
          Dan Poltawski added a comment -

          Thanks Sam, i've integrated this now.

          Fred made some good points, especially I think the group suggestion is a good one. But I don't think it is necessary to delay this landing.

          Show
          Dan Poltawski added a comment - Thanks Sam, i've integrated this now. Fred made some good points, especially I think the group suggestion is a good one. But I don't think it is necessary to delay this landing.
          Hide
          Marina Glancy added a comment -

          Sam, can we make the dropdown menu disappear if user clicks on this button again?

          Show
          Marina Glancy added a comment - Sam, can we make the dropdown menu disappear if user clicks on this button again?
          Hide
          Dan Poltawski added a comment -

          Bah! Sam, the icon sizing issue mentioned above is still there!

          Show
          Dan Poltawski added a comment - Bah! Sam, the icon sizing issue mentioned above is still there!
          Hide
          Jason Fowler added a comment -

          Marina: that was discussed a few times during the planning phase of the issue, and a lot of users still double click things on the web (which is why we should disable form submit buttons once they have been clicked) so making this a click-to-show, click-to-hide button is not a great idea.

          Show
          Jason Fowler added a comment - Marina: that was discussed a few times during the planning phase of the issue, and a lot of users still double click things on the web (which is why we should disable form submit buttons once they have been clicked) so making this a click-to-show, click-to-hide button is not a great idea.
          Hide
          Tim Hunt added a comment -

          Well, JavaScript can detect double-clicks.

          So, you can make it that click, or double-click shows. Next click, or double-click hides. It's a bit tricky, and might involve time-outs, but it is probably worth investing the effort for our users.

          Still, since this has been integrated, that now needs to be done as a new issue.

          Show
          Tim Hunt added a comment - Well, JavaScript can detect double-clicks. So, you can make it that click, or double-click shows. Next click, or double-click hides. It's a bit tricky, and might involve time-outs, but it is probably worth investing the effort for our users. Still, since this has been integrated, that now needs to be done as a new issue.
          Hide
          Jason Fowler added a comment -

          I think it should be discussed further before deciding it should be done at all. There is already functionality to hide the menu if you click anywhere outside the menu/icon, and while I don't mind if the icon can be used to hide the menu, I think it should go through thorough discussion before something like that is decided.

          Show
          Jason Fowler added a comment - I think it should be discussed further before deciding it should be done at all. There is already functionality to hide the menu if you click anywhere outside the menu/icon, and while I don't mind if the icon can be used to hide the menu, I think it should go through thorough discussion before something like that is decided.
          Hide
          Frédéric Massart added a comment -

          Are double-clicks Mobile compliant?

          Show
          Frédéric Massart added a comment - Are double-clicks Mobile compliant?
          Hide
          Tim Hunt added a comment -

          I don't know about you Fred, but I don't have a mouse plugged in to my mobile phone. I can double-tap on the screen though.

          Show
          Tim Hunt added a comment - I don't know about you Fred, but I don't have a mouse plugged in to my mobile phone. I can double-tap on the screen though.
          Hide
          Frédéric Massart added a comment -

          This is what I had in mind when I mentioned the dropdown before the activity name (actually it is better before the activity icon).

          Along with that, I would add a small "Handle" for the activity "Move" which could only be visible on hover, and the "edit title" on the right of the activity name.

          Show
          Frédéric Massart added a comment - This is what I had in mind when I mentioned the dropdown before the activity name (actually it is better before the activity icon). Along with that, I would add a small "Handle" for the activity "Move" which could only be visible on hover, and the "edit title" on the right of the activity name.
          Hide
          Frédéric Massart added a comment -

          Sure Tim, but that's not a common practice on Touch devices AFAIK. I think it's more of a zoom-in function actually (in a Mobile browser).

          Show
          Frédéric Massart added a comment - Sure Tim, but that's not a common practice on Touch devices AFAIK. I think it's more of a zoom-in function actually (in a Mobile browser).
          Hide
          Jason Fowler added a comment -

          Placing the dropdown before the activity leads to things moving when the edit on/off is toggled. I think that is why it has traditionally been at the end of the activity name. I would like to see it right aligned, with some way of indicating which activity it is attached to that works on all devices.

          Show
          Jason Fowler added a comment - Placing the dropdown before the activity leads to things moving when the edit on/off is toggled. I think that is why it has traditionally been at the end of the activity name. I would like to see it right aligned, with some way of indicating which activity it is attached to that works on all devices.
          Hide
          Frédéric Massart added a comment -

          I don't see any problem with that, it's a different page anyway, with a slightly different layout. Right aligned causes some issues when the activity name is quite long, also it can get close to the activity completion status (which could be moved too actually).

          Show
          Frédéric Massart added a comment - I don't see any problem with that, it's a different page anyway, with a slightly different layout. Right aligned causes some issues when the activity name is quite long, also it can get close to the activity completion status (which could be moved too actually).
          Hide
          Tim Hunt added a comment -

          Fred, what we are talking about is that if we implement tap again to close, we ignore double taps in that. I don't see a problem there for mobile devices. It would be different if we were planning to implment something that could only be done with double-tap.

          Show
          Tim Hunt added a comment - Fred, what we are talking about is that if we implement tap again to close, we ignore double taps in that. I don't see a problem there for mobile devices. It would be different if we were planning to implment something that could only be done with double-tap.
          Hide
          Martin Dougiamas added a comment -

          I still think we need a user preference for this. And a new user preferences page to put it on.

          Show
          Martin Dougiamas added a comment - I still think we need a user preference for this. And a new user preferences page to put it on.
          Hide
          Michael de Raadt added a comment -

          I ran the unit tests and Behat tests successfully after this change. There were some issues in Windows, but they were unrelated.

          The general functionality works on the course page and on blocks.

          I found a flaw in the activities in the Main menu block on the Front page. The menu for the resource/activities in this block were truncated or squashed. The rename function does not work in this block.

          When I fixed the group mode, there was no tool-tip that indicated the meaning of the fixed icon.

          I'm not sure I like this change. I understand the rationale for keeping the status icons out of the menu, but it seems visually odd to include some icons but not others. The styling of the menu, including padding and shadowing, differs from other dialogues in Moodle.

          Show
          Michael de Raadt added a comment - I ran the unit tests and Behat tests successfully after this change. There were some issues in Windows, but they were unrelated. The general functionality works on the course page and on blocks. I found a flaw in the activities in the Main menu block on the Front page. The menu for the resource/activities in this block were truncated or squashed. The rename function does not work in this block. When I fixed the group mode, there was no tool-tip that indicated the meaning of the fixed icon. I'm not sure I like this change. I understand the rationale for keeping the status icons out of the menu, but it seems visually odd to include some icons but not others. The styling of the menu, including padding and shadowing, differs from other dialogues in Moodle.
          Hide
          Michael de Raadt added a comment - - edited

          I tested this on mobile also (Android, Chrome). The new menu seemed to move the completion icon.

          Actually, this seems to have been a problem before this menu came along, but it would have been nice to resolve this if we are reducing the space needed for icons.

          Show
          Michael de Raadt added a comment - - edited I tested this on mobile also (Android, Chrome). The new menu seemed to move the completion icon. Actually, this seems to have been a problem before this menu came along, but it would have been nice to resolve this if we are reducing the space needed for icons.
          Hide
          Jason Fowler added a comment -

          I was told that Sam would look in to the alignment of the completion icon while working on this issue, it was originally part of the "general CSS clean up for Clean" issue that also included the bold labels etc.

          Show
          Jason Fowler added a comment - I was told that Sam would look in to the alignment of the completion icon while working on this issue, it was originally part of the "general CSS clean up for Clean" issue that also included the bold labels etc.
          Hide
          Andrew Nicols added a comment -

          Another issue I've just noticed on this is that it does not apply the JS features to newly drag/dropped files.

          There are some registration and invocation functions in moodle-course-coursebase which handle this. Toolbox (used to) register. Drag/Drop invokes.

          Show
          Andrew Nicols added a comment - Another issue I've just noticed on this is that it does not apply the JS features to newly drag/dropped files. There are some registration and invocation functions in moodle-course-coursebase which handle this. Toolbox (used to) register. Drag/Drop invokes.
          Hide
          Mary Evans added a comment - - edited

          Just a thought regarding the Editing Icons within the blocks themselves. If you take a look at Boxxie theme I had to fix the editing controls in the side blocks because the grey icons did not stand out well enough. However after adding padding and a border and a lighter background to these icons, they not only stand out but are bigger, and I suspect easier to access via a touch screen, and as such may actually fix this editing problem for the blocks.

          The code for this was first mooted here...
          https://moodle.org/mod/forum/discuss.php?d=175572#p769966

          Show
          Mary Evans added a comment - - edited Just a thought regarding the Editing Icons within the blocks themselves. If you take a look at Boxxie theme I had to fix the editing controls in the side blocks because the grey icons did not stand out well enough. However after adding padding and a border and a lighter background to these icons, they not only stand out but are bigger, and I suspect easier to access via a touch screen, and as such may actually fix this editing problem for the blocks. The code for this was first mooted here... https://moodle.org/mod/forum/discuss.php?d=175572#p769966
          Hide
          Andrew Nicols added a comment -

          e.g. ajax show/hide is broken for starters.
          There are also some CSS issues on standard theme. The mouse-over on the menu isn't quite right.

          Show
          Andrew Nicols added a comment - e.g. ajax show/hide is broken for starters. There are also some CSS issues on standard theme. The mouse-over on the menu isn't quite right.
          Hide
          Andrew Nicols added a comment -

          And it seems that when closing the menu, the section editing cog gets resized or loses it's icon-small class.

          Show
          Andrew Nicols added a comment - And it seems that when closing the menu, the section editing cog gets resized or loses it's icon-small class.
          Hide
          Sam Hemelryk added a comment -

          Hi all, wow feedbacks gone wild since this landed to integration.

          I've produced a patch now that makes the following changes:

          1. course_section_cm_edit_actions now allows the caller more options for the display of the action menu.
          2. The site menu block now disables the JS enhancement of the actionmenu so that it displays as it did before. After more testing this seemed like the only option that worked in the limited space of the block when it had several modules.
          3. draganddrop of resources now triggers the action menu enhancement as well.
          4. Fixed display of title when the menu is not being enhanced by JS.
          5. Fixed the alignment of the completion icon in the bootstrapbase theme.
          6. Tweaked the CSS for the standard theme.

          Repo: git://github.com/samhemelryk/moodle.github
          Branch: wip-MDL-39814-m26-p1
          Diff: https://github.com/samhemelryk/moodle/commit/wip-MDL-39814-m26-p1

          Michael in regards to the points you raised:

          1. The rename function in that block has always redirected rather than allowing editing in place. When you say it doesn't work do you mean that you don't get redirected or that you expected to be able to edit in place?
          2. You say when you fixed the group mode, what did you have to fix there?
          3. A new issue will be opened after this gets integrated to discuss what icons go in the menu and what do not.

          Andrew in regards to your points:

          1. Thanks for raising that this doesnt' integrate nicely with drag and drop resources. I've fixed this now in the patch noted above. (Big thanks for pointing out the area of code that required addressing)
          2. The ajax show hide worked fine for me, I couldn't reproduce what any error sorry.
          3. I also couldn't find any change in the section cog when the menu was opened and closed. Any chance you could check you are in fact still seeing these issues?

          New issues to come after this gets in:

          1. Open a new issue to sort out which icons should be displayed and which should not.
          2. Open a new issue to consider closing the popup on a second click.
          3. Open a new issue to add a user preferences page and make a preference to toogle this on/off.

          When another integrator comes online I'll discuss the how this issue will be handled.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi all, wow feedbacks gone wild since this landed to integration. I've produced a patch now that makes the following changes: course_section_cm_edit_actions now allows the caller more options for the display of the action menu. The site menu block now disables the JS enhancement of the actionmenu so that it displays as it did before. After more testing this seemed like the only option that worked in the limited space of the block when it had several modules. draganddrop of resources now triggers the action menu enhancement as well. Fixed display of title when the menu is not being enhanced by JS. Fixed the alignment of the completion icon in the bootstrapbase theme. Tweaked the CSS for the standard theme. Repo: git://github.com/samhemelryk/moodle.github Branch: wip- MDL-39814 -m26-p1 Diff: https://github.com/samhemelryk/moodle/commit/wip-MDL-39814-m26-p1 Michael in regards to the points you raised: The rename function in that block has always redirected rather than allowing editing in place. When you say it doesn't work do you mean that you don't get redirected or that you expected to be able to edit in place? You say when you fixed the group mode, what did you have to fix there? A new issue will be opened after this gets integrated to discuss what icons go in the menu and what do not. Andrew in regards to your points: Thanks for raising that this doesnt' integrate nicely with drag and drop resources. I've fixed this now in the patch noted above. (Big thanks for pointing out the area of code that required addressing) The ajax show hide worked fine for me, I couldn't reproduce what any error sorry. I also couldn't find any change in the section cog when the menu was opened and closed. Any chance you could check you are in fact still seeing these issues? New issues to come after this gets in: Open a new issue to sort out which icons should be displayed and which should not. Open a new issue to consider closing the popup on a second click. Open a new issue to add a user preferences page and make a preference to toogle this on/off. When another integrator comes online I'll discuss the how this issue will be handled. Many thanks Sam
          Hide
          Damyon Wiese added a comment -

          Re: user preference - for people upgrading who like the old way better - this would be useful - but I'm not sure how you would explain the point of a setting for this to a new teacher/student.

          Show
          Damyon Wiese added a comment - Re: user preference - for people upgrading who like the old way better - this would be useful - but I'm not sure how you would explain the point of a setting for this to a new teacher/student.
          Hide
          Damyon Wiese added a comment -

          I've pulled those changes in Sam and bumped the version number.

          Back to testing...

          Thanks!

          Show
          Damyon Wiese added a comment - I've pulled those changes in Sam and bumped the version number. Back to testing... Thanks!
          Hide
          Paul Nicholls added a comment -

          Damyon's comment re: user preference makes me wonder - would it be better as a site-wide setting? It strikes me that some administrators may wish to keep their entire site using the "old way", either because of their theme (i.e. Decaf, which does something similar itself but which I imagine - I haven't tried yet - would require either substantial reworking or custom renderers which mimic the "old way" in order to handle this change) or because they don't want to deal with the training implications (which may be rather higher than you'd expect, in places with non-tech-savvy teaching staff). Perhaps there could still be a user preference too, but only available if the site-wide setting has been enabled...

          Show
          Paul Nicholls added a comment - Damyon's comment re: user preference makes me wonder - would it be better as a site-wide setting? It strikes me that some administrators may wish to keep their entire site using the "old way", either because of their theme (i.e. Decaf, which does something similar itself but which I imagine - I haven't tried yet - would require either substantial reworking or custom renderers which mimic the "old way" in order to handle this change) or because they don't want to deal with the training implications (which may be rather higher than you'd expect, in places with non-tech-savvy teaching staff). Perhaps there could still be a user preference too, but only available if the site-wide setting has been enabled...
          Hide
          Sam Hemelryk added a comment -

          Hi Paul,
          This work includes two settings that can be used to disable the new menu for modules and blocks respectively.
          $CFG->modeditingmenu on by default, turn it off and you'll get the old style activity editing icons.
          $CFG->blockeditingmenu on by default, same as above but for blocks.

          Show
          Sam Hemelryk added a comment - Hi Paul, This work includes two settings that can be used to disable the new menu for modules and blocks respectively. $CFG->modeditingmenu on by default, turn it off and you'll get the old style activity editing icons. $CFG->blockeditingmenu on by default, same as above but for blocks.
          Hide
          Michael de Raadt added a comment -

          Test result: Success

          I think the critical problems have been resolved.

          Two further follow-up issues (and I suspect this issue will generate a few more still):

          • Show the tooltip on the group icon when group mode is forced in a course
          • Fix the positioning of the completion icon when screen size gets small (not a regression from this issue, but discovered during testing)
          Show
          Michael de Raadt added a comment - Test result: Success I think the critical problems have been resolved. Two further follow-up issues (and I suspect this issue will generate a few more still): Show the tooltip on the group icon when group mode is forced in a course Fix the positioning of the completion icon when screen size gets small (not a regression from this issue, but discovered during testing)
          Hide
          Paul Nicholls added a comment -

          Ah, that sounds fine, Sam - I've been trying to keep up with the comments, but haven't had time to look at the code or try it out myself.

          Show
          Paul Nicholls added a comment - Ah, that sounds fine, Sam - I've been trying to keep up with the comments, but haven't had time to look at the code or try it out myself.
          Hide
          Sam Hemelryk added a comment -

          Thanks Michael,

          I've created the required issues now:

          • MDL-40973 - completion icon display messed up on small screens when using clean theme.
          • MDL-40974 - fixed group icon tooltip isn't being displayed.
          • MDL-40975 - which icons should be shown in the menu and which shouldn't.
          • MDL-40976 - consider closing the action menu dropdown if the open icon is clicked for a second time.
          • MDL-40977 - add a user preference and a display page for perferences.

          Many thanks to everyone involved - please if you've got an interest in the new issues go and comment there so that we can finalise things regarding this menu and get it perfect before the release of 2.6

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Michael, I've created the required issues now: MDL-40973 - completion icon display messed up on small screens when using clean theme. MDL-40974 - fixed group icon tooltip isn't being displayed. MDL-40975 - which icons should be shown in the menu and which shouldn't. MDL-40976 - consider closing the action menu dropdown if the open icon is clicked for a second time. MDL-40977 - add a user preference and a display page for perferences. Many thanks to everyone involved - please if you've got an interest in the new issues go and comment there so that we can finalise things regarding this menu and get it perfect before the release of 2.6 Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Against all probability we've achieved normality. You changes didn't break the tests I pretended to run and are now immortalised upstream. Good for you!

          "It was a programming technique that had been reverse-engineered from the sort of psychotic mental blocks that otherwise perfectly normal people had been observed invariably to develop when elected to high political office."
          Adams, D (1992) Mostly Harmless. London: William Heinemann.

          Show
          Sam Hemelryk added a comment - Against all probability we've achieved normality. You changes didn't break the tests I pretended to run and are now immortalised upstream. Good for you! "It was a programming technique that had been reverse-engineered from the sort of psychotic mental blocks that otherwise perfectly normal people had been observed invariably to develop when elected to high political office." Adams, D (1992) Mostly Harmless. London: William Heinemann.
          Hide
          Marina Glancy added a comment -

          FYI we found a regression in moving the module between visible and hidden sections: MDL-41227
          It is most likely (but not 100%) caused by this issue

          Show
          Marina Glancy added a comment - FYI we found a regression in moving the module between visible and hidden sections: MDL-41227 It is most likely (but not 100%) caused by this issue
          Hide
          Frédéric Massart added a comment -

          I have to say that I am still not sold on this feature. To me this is more confusing users than it helps solving the usability issues on Mobile. I would assume that most users are not used to use Moodle on Mobiles and would instinctively use a desktop. We have to make sure we are not affecting the usability on desktop for the only purpose of being more usable on Mobile.

          Have we posted on the forums to gather some feedback from users? If not, I find it strange that we do so for specs, but not for things that will affect a much larger pool of users.

          Show
          Frédéric Massart added a comment - I have to say that I am still not sold on this feature. To me this is more confusing users than it helps solving the usability issues on Mobile. I would assume that most users are not used to use Moodle on Mobiles and would instinctively use a desktop. We have to make sure we are not affecting the usability on desktop for the only purpose of being more usable on Mobile. Have we posted on the forums to gather some feedback from users? If not, I find it strange that we do so for specs, but not for things that will affect a much larger pool of users.
          Hide
          Marina Glancy added a comment -

          yes, I would like this to be optional as well. BTW recently I noticed that menu does not disappear after I change indentation (choose > or < )

          Show
          Marina Glancy added a comment - yes, I would like this to be optional as well. BTW recently I noticed that menu does not disappear after I change indentation (choose > or < )
          Hide
          Damyon Wiese added a comment -

          It is optional (site-wide).

          Show
          Damyon Wiese added a comment - It is optional (site-wide).
          Hide
          Sam Hemelryk added a comment - - edited

          Yip, Damyon is right this is can be disabled at the site level.

          Also worth noting that this was discussed (to death) in the forums and on several other issues on tracker.
          It has been purposed dozens of times before, code has been written several times, and finally this one got through.
          Is it to everyone's tastes, surely not, but it has been a very popular topic in the scheme of things.
          Fingers crossed it gets a warm reception, if not I'm sure it will continue to develop.

          Show
          Sam Hemelryk added a comment - - edited Yip, Damyon is right this is can be disabled at the site level. Also worth noting that this was discussed (to death) in the forums and on several other issues on tracker. It has been purposed dozens of times before, code has been written several times, and finally this one got through. Is it to everyone's tastes, surely not, but it has been a very popular topic in the scheme of things. Fingers crossed it gets a warm reception, if not I'm sure it will continue to develop.
          Hide
          Marina Glancy added a comment -

          Ok, finally found a setting. Thanks
          I created an issue about spacing and menu that stays open: MDL-41504

          Show
          Marina Glancy added a comment - Ok, finally found a setting. Thanks I created an issue about spacing and menu that stays open: MDL-41504
          Hide
          Mary Evans added a comment -

          Last time I looked, this seem to break Site forum news icons in Main Menu block in Afterburner. The icons are all over the place. Not sure why? Also the sprite image, used in the side blocks (right side only) bleeds into the gap at the bottom of the previous block.

          Just thought I would let you know, I'll open a new tracker for this.

          Other than that it looks good!

          Thanks
          Mary

          Show
          Mary Evans added a comment - Last time I looked, this seem to break Site forum news icons in Main Menu block in Afterburner. The icons are all over the place. Not sure why? Also the sprite image, used in the side blocks (right side only) bleeds into the gap at the bottom of the previous block. Just thought I would let you know, I'll open a new tracker for this. Other than that it looks good! Thanks Mary
          Hide
          Mary Cooch added a comment -

          Removing docs_require label as I have documented this here: http://docs.moodle.org/26/en/Course_homepage

          Show
          Mary Cooch added a comment - Removing docs_require label as I have documented this here: http://docs.moodle.org/26/en/Course_homepage
          Hide
          Mary Cooch added a comment -

          Adding docs_required back to remind me there might be more changes

          Show
          Mary Cooch added a comment - Adding docs_required back to remind me there might be more changes
          Hide
          Martin Dougiamas added a comment -

          Important follow-up work is happening here: MDL-40975

          Show
          Martin Dougiamas added a comment - Important follow-up work is happening here: MDL-40975

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              25 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile