Moodle
  1. Moodle
  2. MDL-38012 META: Simplify moodle forms
  3. MDL-38681

Change expand all / collapse all buttons from buttons to links

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide
      1. Go to a course page settings
      2. Make sure the collapse/expand all link works as expected
      3. Make sure you can "Collapse all", when ALL the section are expanded, otherwise the option is "Expand all"
      4. Play with the attached file to test multiple forms and other stuff
      Show
      Go to a course page settings Make sure the collapse/expand all link works as expected Make sure you can "Collapse all", when ALL the section are expanded, otherwise the option is "Expand all" Play with the attached file to test multiple forms and other stuff
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38681-master-int
    • Rank:
      48730

      Description

      The expand all/collapse all buttons look too much like the form submission buttons (which should be the most obvious buttons in the page).

      IMO they should be changed to links.

      1. test_forms.php
        9 kB
        Frédéric Massart
      1. bonfire-screenshot-20130325-162338-650.png
        28 kB
      2. Collapsed.jpg
        28 kB

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          I vote for this and also make them the same as similar links in courses listings (MDL-37009). We can move the buttons for course listings on top as well

          Show
          Marina Glancy added a comment - I vote for this and also make them the same as similar links in courses listings ( MDL-37009 ). We can move the buttons for course listings on top as well
          Hide
          David Mudrak added a comment -

          +1 for links. Just a quick note. If it is really implemented via <a> elements, please make sure that (assuming it is purely generated by javascript progressive enhancement) the default link's behaviour is disabled and the event propagation is halted (http://yuilibrary.com/yui/docs/event/#epreventdefault-and-estoppropagation). In other words, nothing should happen if the user invokes "Open link in a new window/tab" in their browser (by middle-button mouse click, for example).

          Show
          David Mudrak added a comment - +1 for links. Just a quick note. If it is really implemented via <a> elements, please make sure that (assuming it is purely generated by javascript progressive enhancement) the default link's behaviour is disabled and the event propagation is halted ( http://yuilibrary.com/yui/docs/event/#epreventdefault-and-estoppropagation ). In other words, nothing should happen if the user invokes "Open link in a new window/tab" in their browser (by middle-button mouse click, for example).
          Hide
          Tim Hunt added a comment -

          +1 from me too. (I had already requested this as part of the original code-review, but got talked out of it.

          Show
          Tim Hunt added a comment - +1 from me too. (I had already requested this as part of the original code-review, but got talked out of it.
          Hide
          Martin Dougiamas added a comment -

          yeah +1 links, at the top.

          Show
          Martin Dougiamas added a comment - yeah +1 links, at the top.
          Hide
          Helen Foster added a comment -

          Just attaching a screenshot of how expand all / collapse all is done in Gareth B's collapsed topics course format, which looks really clear to me.

          Show
          Helen Foster added a comment - Just attaching a screenshot of how expand all / collapse all is done in Gareth B's collapsed topics course format, which looks really clear to me.
          Hide
          Frédéric Massart added a comment -

          Sending for peer review. I based the logic on Gmail's.

          • There is only one option at a time, no need to pollute the interface with 2 links, when one is required
          • To collapse all, all the sections should be expanded
          • The links are right aligned not to disrupt the user who do not need to know that an expand all option is possible. He will find it if he needs it
          Show
          Frédéric Massart added a comment - Sending for peer review. I based the logic on Gmail's. There is only one option at a time, no need to pollute the interface with 2 links, when one is required To collapse all, all the sections should be expanded The links are right aligned not to disrupt the user who do not need to know that an expand all option is possible. He will find it if he needs it
          Hide
          Marina Glancy added a comment -

          Maybe Andrew Nicols can review it?

          Show
          Marina Glancy added a comment - Maybe Andrew Nicols can review it?
          Hide
          Jason Fowler added a comment - - edited

          I went through an issue that called for the expand icons to be changed from links to buttons for accessibility reasons. Is this such a good idea? What I mean is, they could still be buttons, just formatted to look like links...

          Show
          Jason Fowler added a comment - - edited I went through an issue that called for the expand icons to be changed from links to buttons for accessibility reasons. Is this such a good idea? What I mean is, they could still be buttons, just formatted to look like links...
          Hide
          Tim Hunt added a comment -

          Jason, it would be helpful if you gave the issue id of the other issue.

          Show
          Tim Hunt added a comment - Jason, it would be helpful if you gave the issue id of the other issue.
          Hide
          Tim Hunt added a comment -

          Fred, it seems odd to change the button to a link using JavaScript. Am I missing some obvious reason why you can't just output an <a href="#" ...> in the PHP code?

          Show
          Tim Hunt added a comment - Fred, it seems odd to change the button to a link using JavaScript. Am I missing some obvious reason why you can't just output an <a href="#" ...> in the PHP code?
          Hide
          Andrew Nicols added a comment -

          It would be good if you could explain why you're converting the span to an anchor - just for future reference when 6 months later someone tries to work it out.

          [N] Syntax
          [N] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [Y] Security
          [-] Documentation
          [N] Git
          [Y] Sanity check

          Syntax:

          Git

          Commit message is a statement of fact and doesn't say what you've done. Not sure if it's "wrong", but it doesn't feel right...

          Also, it would be helpful if you could briefly explain what you've done in the commit message.

          Output

          The existing span should only be visible if the jsenabled class is present on the body.

          Otherwise, looks good to me and works as expected. Please feel free to submit for integration.

          Show
          Andrew Nicols added a comment - It would be good if you could explain why you're converting the span to an anchor - just for future reference when 6 months later someone tries to work it out. [N] Syntax [N] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [Y] Security [-] Documentation [N] Git [Y] Sanity check Syntax: When you convert the span to an anchor, is there any reason that you can't add a new anchor and move the existing span to be a child in that anchor? // Make the collapse/expand a link. btn = Y.one(SELECTORS.COLLAPSEEXPAND); link = Y.Node.create('<a href= "#" ></a>'); btn.get('parentNode').insertBefore(link); link.appendChild(btn); It'd be grand if you could fix https://github.com/FMCorz/moodle/compare/18ceea60afa4c4c8d5a30c4852aa121db36f905c...MDL-38681-master-int#L0R81 - I know it's not touched by this issue, but that + on SELECTORS.FIELDSETCOLLAPSIBLE is wrong. collapsed isn't defined before use now you've taken it out of the args list - https://github.com/FMCorz/moodle/compare/18ceea60afa4c4c8d5a30c4852aa121db36f905c...MDL-38681-master-int#L0R119 https://github.com/FMCorz/moodle/compare/18ceea60afa4c4c8d5a30c4852aa121db36f905c...MDL-38681-master-int#L0R158 should use M.util.get_string rather than accessing the string cache directly CSS should be in expanded form rather than as a one-liner Git Commit message is a statement of fact and doesn't say what you've done. Not sure if it's "wrong", but it doesn't feel right... Also, it would be helpful if you could briefly explain what you've done in the commit message. Output The existing span should only be visible if the jsenabled class is present on the body. Otherwise, looks good to me and works as expected. Please feel free to submit for integration.
          Hide
          Jason Fowler added a comment -

          MDL-35825 was the issue where the "Expandable groups in tree navigation blocks should use button tags not link tags" problem was raised.

          Show
          Jason Fowler added a comment - MDL-35825 was the issue where the "Expandable groups in tree navigation blocks should use button tags not link tags" problem was raised.
          Hide
          Frédéric Massart added a comment -

          Thanks for your review Andrew,

          Why span to link?

          There are multiple reasons why I decided to proceed that way:

          • I didn't want the link to appear clickable before the Javascript is loaded
            • Either it'd lead to a page refresh
            • Or it would not appear to work as no action associated yet
            • Showing the link when the JS is loaded creates a flickering that I really don't like!
          • For it to be properly stylable, without introducing more classes I thought that it was better not to add the span into the anchor. Especially that the anchor would JS generated and so modifying the DOM structure which would confuse themers.

          I'm happy to update that if voted against.

          The rest

          • Created a new selector for the combination of selector and class
          • Good spotting, collapsed is now declared with var
          • The CSS matches the style of the rest of the document, that's why it's inline
          • Use get_string instead of M.str; Thanks, I knew I was doing something wrong there.
          • Rephrased the commit message a bit
          • The span was already hidden by CSS (see .jsenabled .collapsible-actions)

          Pushing for integration. Thanks!

          Show
          Frédéric Massart added a comment - Thanks for your review Andrew, Why span to link? There are multiple reasons why I decided to proceed that way: I didn't want the link to appear clickable before the Javascript is loaded Either it'd lead to a page refresh Or it would not appear to work as no action associated yet Showing the link when the JS is loaded creates a flickering that I really don't like! For it to be properly stylable, without introducing more classes I thought that it was better not to add the span into the anchor. Especially that the anchor would JS generated and so modifying the DOM structure which would confuse themers. I'm happy to update that if voted against. The rest Created a new selector for the combination of selector and class Good spotting, collapsed is now declared with var The CSS matches the style of the rest of the document, that's why it's inline Use get_string instead of M.str; Thanks, I knew I was doing something wrong there. Rephrased the commit message a bit The span was already hidden by CSS (see .jsenabled .collapsible-actions) Pushing for integration. Thanks!
          Hide
          Damyon Wiese added a comment -

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

          Thanks!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
          Hide
          Dan Poltawski added a comment -

          Integrated to master. Thanks Fred.

          Show
          Dan Poltawski added a comment - Integrated to master. Thanks Fred.
          Hide
          Adrian Greeve added a comment -

          Tested on the master integration branch.
          I was testing this under the boot strap theme and found that the show more link wasn't working, but I think that this is an issue with that theme as it works fine in the standard theme.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the master integration branch. I was testing this under the boot strap theme and found that the show more link wasn't working, but I think that this is an issue with that theme as it works fine in the standard theme. Test passed.
          Hide
          Dan Poltawski added a comment -

          Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

          line 1289 of \lib\changes.php: call to debugging()
          line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
          line 202 of \lib\now.php: call to moodleform->_is_poor_form()
          line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

          Show
          Dan Poltawski added a comment - Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking. line 1289 of \lib\changes.php: call to debugging() line 281 of \lib\are.php: call to moodleform->detectMissingThanks() line 202 of \lib\now.php: call to moodleform->_is_poor_form() line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

            People

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

              Dates

              • Created:
                Updated:
                Resolved: