Moodle
  1. Moodle
  2. MDL-30641

Filepicker lists folders with <i><u>...</u></i> when in 'View as list' mode

    Details

    • Testing Instructions:
      Hide

      Requirements

      These instructions require a repository with a sub folder. I used the 'Server files' repository, but any will do.

      Instructions

      • open a course, turn editing mode on, and add a page;
      • under the description, choose the 'moodle media' button;
      • choose 'Find or upload a sound, video or applet...';
      • when the filepicker opens, choose 'View as list'; and
      • navigate to a repository with folders within it (e.g. Server Files).

      Expected Result

      A list of folder names

      Actual Result

      A list folder names wrapped in <i><u> tags

      Further instructions

      • now select 'View as icons' again; and
      • mouse over one of the folders.

      Expected Result

      The folders should not be italicised or underlined
      The anchor title is the filename

      Actual Result

      The folders are italicised and underlined
      The anchor title is set to the <i><u> text

      Show
      Requirements These instructions require a repository with a sub folder. I used the 'Server files' repository, but any will do. Instructions open a course, turn editing mode on, and add a page; under the description, choose the 'moodle media' button; choose 'Find or upload a sound, video or applet...'; when the filepicker opens, choose 'View as list'; and navigate to a repository with folders within it (e.g. Server Files). Expected Result A list of folder names Actual Result A list folder names wrapped in <i><u> tags Further instructions now select 'View as icons' again; and mouse over one of the folders. Expected Result The folders should not be italicised or underlined The anchor title is the filename Actual Result The folders are italicised and underlined The anchor title is set to the <i><u> text
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30641-master-1
    • Rank:
      33455

      Description

      When using the file picker and viewing items using the 'View as list' option, any folders are displayed as
      <i><u>TITLE</u></i>

      After viewing as 'View as list', changing back to 'View as icons' gives similar results for the anchor titles, and the filename of the folders

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Also noticed that when using 'View as Icons', the anchor title contains the same tags

          Show
          Andrew Nicols added a comment - Also noticed that when using 'View as Icons', the anchor title contains the same tags
          Hide
          Andrew Nicols added a comment -

          I'm not convinced of the benefits of italicising, or underlining the folders at all.

          The patch I've attached removes the code which does so and thus resolves the problem.

          If this is desired, then appropriate css tags should be added and it should really be done with CSS allowing theme designers to disagree amongst one another

          Show
          Andrew Nicols added a comment - I'm not convinced of the benefits of italicising, or underlining the folders at all. The patch I've attached removes the code which does so and thus resolves the problem. If this is desired, then appropriate css tags should be added and it should really be done with CSS allowing theme designers to disagree amongst one another
          Hide
          Andrew Nicols added a comment -

          This patch cherry-picks cleanly to Moodle 2.1

          Show
          Andrew Nicols added a comment - This patch cherry-picks cleanly to Moodle 2.1
          Hide
          Andrew Nicols added a comment -

          I've also pushed another commit MDL-30641-master-2 which modifies the way that the file picker presents the elements

          For the icons format, it uses the YUI3 Node.create function rather than document.createElement which means that it's no longer necessary to grab the element name by creating an id, and manipulating it as it's possible to add the listeners directly to the YUI3 Node object. I've also modified the HTML tree to wrap the entire file/folder element in one anchor, rather than having multiple anchors each with their own listener.

          For the list format, I've simply added a class of folder/file to the tree node so that it should be possble to use CSS such as:

          div.fp-tree-panel table.folder {
            font-weight: bold;
          }
          
          div.fp-tree-panel table.file {
            font-style: italic;
          }
          

          I suspect that this change is too large for the 2.1 and 2.2 series, but it might be considered for master

          Show
          Andrew Nicols added a comment - I've also pushed another commit MDL-30641 -master-2 which modifies the way that the file picker presents the elements For the icons format, it uses the YUI3 Node.create function rather than document.createElement which means that it's no longer necessary to grab the element name by creating an id, and manipulating it as it's possible to add the listeners directly to the YUI3 Node object. I've also modified the HTML tree to wrap the entire file/folder element in one anchor, rather than having multiple anchors each with their own listener. For the list format, I've simply added a class of folder/file to the tree node so that it should be possble to use CSS such as: div.fp-tree-panel table.folder { font-weight: bold; } div.fp-tree-panel table.file { font-style: italic; } I suspect that this change is too large for the 2.1 and 2.2 series, but it might be considered for master
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,
          Normally Dongsheng would peer-review filepicker issues, however he is away at the moment so I'll pick this up.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Normally Dongsheng would peer-review filepicker issues, however he is away at the moment so I'll pick this up. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, changes look spot on!
          Feel free to put up for integration when you're ready.

          Show
          Sam Hemelryk added a comment - Thanks Andrew, changes look spot on! Feel free to put up for integration when you're ready.
          Hide
          Sam Hemelryk added a comment -

          I forgot to mention, I also had a quick look at the MDL-30641-master-2 branch.
          While I didn't review it properly the changes that you are making there look good, and certainly its a brilliant idea.
          I think that certainly it should be split off onto a separate issue. It is an improvement but it would get a +1 from me for master & MOODLE_22_STABLE.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - I forgot to mention, I also had a quick look at the MDL-30641 -master-2 branch. While I didn't review it properly the changes that you are making there look good, and certainly its a brilliant idea. I think that certainly it should be split off onto a separate issue. It is an improvement but it would get a +1 from me for master & MOODLE_22_STABLE. Cheers Sam
          Hide
          Andrew Nicols added a comment -

          Just to confirm, this should cherry-pick cleanly to:

          • master
          • MOODLE_22_STABLE
          • MOODLE_21_STABLE
          Show
          Andrew Nicols added a comment - Just to confirm, this should cherry-pick cleanly to: master MOODLE_22_STABLE MOODLE_21_STABLE
          Hide
          Aparup Banerjee added a comment -

          Thanks , this has been integrated into master and picked into 22 and 21 branches

          tester: remember to purge caches

          Show
          Aparup Banerjee added a comment - Thanks , this has been integrated into master and picked into 22 and 21 branches tester: remember to purge caches
          Hide
          Ankit Agarwal added a comment -

          this is working great!

          Show
          Ankit Agarwal added a comment - this is working great!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

          Now... disconnect, relax and enjoy the next days, yay!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: