Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.5
    • Component/s: Hub
    • Labels:
    • Testing Instructions:
      Hide

      Add community finder block. Click on the search link.

      Check that when

      js is enabled:

      • you see a fancy and working hub selector
      • search still work
      • most of search fields are hidden by advanced button
      • "sort by" field is now a select field
      • In the search result, Download button is been renamed Install

      js is disabled:

      • you still can search.
      Show
      Add community finder block. Click on the search link. Check that when js is enabled: you see a fancy and working hub selector search still work most of search fields are hidden by advanced button "sort by" field is now a select field In the search result, Download button is been renamed Install js is disabled: you still can search.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-37012-master
    • Rank:
      46551

      Description

      Improve the community finder search formL

      • sort by should be a select field
      • selecting hub by radio button is very ugly
      • most of the field should be advance field
      • download button should be named upload button

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Hi Jerome,

          I've just seen that you're working on this - awesome

          So I was having a quick look at the JS you've been working on, and although it's great that you're writing it as a YUI module, I'm concerned about some of the performance implications of having that module extend Y.Base. Having an object extend Y.Base is pretty intensive on the whole. You do get some good stuff from it - like the lifecycle control, and attributes, but if you're not using them then there is no bnefit. Generally it's fine for a handful of instances, but being a form element it is completely possible that someone could insert hundreds of listings which would get nasty.

          There's a really interesting talk from YUIConf which covers this - see http://www.youtube.com/watch?v=8cTz73zdDuc&feature=youtube_gdata_player - the relevant part starts at about 14:33.

          Show
          Andrew Nicols added a comment - Hi Jerome, I've just seen that you're working on this - awesome So I was having a quick look at the JS you've been working on, and although it's great that you're writing it as a YUI module, I'm concerned about some of the performance implications of having that module extend Y.Base. Having an object extend Y.Base is pretty intensive on the whole. You do get some good stuff from it - like the lifecycle control, and attributes, but if you're not using them then there is no bnefit. Generally it's fine for a handful of instances, but being a form element it is completely possible that someone could insert hundreds of listings which would get nasty. There's a really interesting talk from YUIConf which covers this - see http://www.youtube.com/watch?v=8cTz73zdDuc&feature=youtube_gdata_player - the relevant part starts at about 14:33.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Andrew,
          thanks for your quick look. Till I read your comment, I believed, probably wrongly, that it was recommended to write YUI3 Moodle module when possible. But apparently extending Y.base (what all YUI3 Moodle module does if I understand when) is very costly in performance. I'll do some profiling and switch to a static module if it's worth it.

          Show
          Jérôme Mouneyrac added a comment - Hi Andrew, thanks for your quick look. Till I read your comment, I believed, probably wrongly, that it was recommended to write YUI3 Moodle module when possible. But apparently extending Y.base (what all YUI3 Moodle module does if I understand when) is very costly in performance. I'll do some profiling and switch to a static module if it's worth it.
          Hide
          Andrew Nicols added a comment -

          Hi Jerome,

          Writing a YUI3 Moodle Module is the correct way to go, but you don't have to have that module extend Y.Base. As they say, there's more than one way to skin a cat. In the presentation I linked to, Ryan mentions about the Tree work they've been working on - here's an example of that code https://github.com/smugmug/yui3/blob/smugmug/build/tree-node/tree-node.js - it uses a YUI module and it doesn't extend Y.Base.

          I was pondering this overnight though, and I wonder whether it may be possible to make your module lazy. That's to say that rather than having so much work done in the initializer, modify your YUI module such than when the module is included, the data is stored in a hash somewhere, but the object delegation (onclick events) are still added. So the first time the form is used is the first time any real work happens.

          I'll try and dig out an example of where we're doing this already in Moodle.

          Andrew

          Show
          Andrew Nicols added a comment - Hi Jerome, Writing a YUI3 Moodle Module is the correct way to go, but you don't have to have that module extend Y.Base. As they say, there's more than one way to skin a cat. In the presentation I linked to, Ryan mentions about the Tree work they've been working on - here's an example of that code https://github.com/smugmug/yui3/blob/smugmug/build/tree-node/tree-node.js - it uses a YUI module and it doesn't extend Y.Base. I was pondering this overnight though, and I wonder whether it may be possible to make your module lazy. That's to say that rather than having so much work done in the initializer, modify your YUI module such than when the module is included, the data is stored in a hash somewhere, but the object delegation (onclick events) are still added. So the first time the form is used is the first time any real work happens. I'll try and dig out an example of where we're doing this already in Moodle. Andrew
          Hide
          Jérôme Mouneyrac added a comment -

          After talking with Andrew it's ok like that (will need more IE8 performance test later). You can peer-review. Cheers.

          Show
          Jérôme Mouneyrac added a comment - After talking with Andrew it's ok like that (will need more IE8 performance test later). You can peer-review. Cheers.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Edit 21/01/13:
          Taking it back (doing my own peer-review as it's been more than a month in peer-review status now - my commit date from the 7th December):

          • phpdoc often don't start with Uppercase and don't end with a dot.
          • error in the css, remove comment and wrong r rules:
            /*background-color: #F7F7F9;
            borde*/r: 1px solid #E1E1E8;
            
          Show
          Jérôme Mouneyrac added a comment - - edited Edit 21/01/13: Taking it back (doing my own peer-review as it's been more than a month in peer-review status now - my commit date from the 7th December): phpdoc often don't start with Uppercase and don't end with a dot. error in the css, remove comment and wrong r rules: /*background-color: #F7F7F9; borde*/r: 1px solid #E1E1E8;
          Hide
          Jérôme Mouneyrac added a comment -

          Fixes done, sending to Fred for peer-review

          Show
          Jérôme Mouneyrac added a comment - Fixes done, sending to Fred for peer-review
          Hide
          Frédéric Massart added a comment -

          Hi Jerome. Hehe, what a nice new interface we get there, it's nice and smooth! Good job! Here are a few comments:

          Block

          1. Why is by default all hubs are hidden? I'm scared that very few people will notice the 'Show all' button.
          2. In non-js mode, the radio button is not aligned with the Hub name.
          3. I suppose it makes more sense to enable the downloadable courses by default.
          4. Are you sure about using getimagesize() on a remote file? If the purpose of this is to check whether the image exists or not, I'd suggest a HEAD request using a curl object.
          5. Can you hardcode the logo to 40x30 because it's a standard? Does this respect the defined proportions on the HUB?
          6. #161: Missing period at the end of the comment.
          7. #164: I don't like the colon appended to the text, but as you didn't introduce that change let's leave it like that!
          8. #171: This was already set before, but as you're editing... the doc_link method is not very extensive and should not be used to append a doc_link to a text, but before. In your case, a white space is used to separate from the text, but that's not a good practice. Not only it does not allow themers to remove it, but switching to RTL might not give the expected result. I'd suggest to manually output the link and use OUTPUT->help_icon() (or other, I haven't checked the code extensively).
          9. #176: Why using onclick when you can set the target manually? Though W3C don't like targets as the user should decide.

          Form element

          1. Missing defined('MOODLE_INTERNAL') or die() in the listing element.
          2. #113 Missing space between foreach and ().
          3. #115 References to the HUB should not be found in this library.
          4. #119 Could you comment on the use of  ? In general I prefer avoiding them.
          5. #122 Just a semi-colon?
          6. #127 IDs have to be unique if you allow for more than one listing element per page.

          JS

          1. We should use the same coding style as in PHP. CamelCase in variable names should not be used.
          2. You can maybe make use of the class .jsenabled (set in body) to display your form instead of using JS to switch it.
          3. In general, I'm not sure about the use of setHTML(). I'd suggest creating a node and setAttrs() on it. And if you're using setContent(), then do not forget to espace it: Y.Escape.html().
          4. #42: Typo.
          5. #46: Non-dynamic selectors should be set a constant to the class so that they're easily identifiable and updatable if need be.
          6. #59: Double ;
          7. #60: Hard to understand where items is defined. Can you make it clearer with a comment or edit?

          General

          • In the language file, I'd define a new string and deprecate the other one if need be instead of changing its meaning.
          • There are quite a few lines which are split to early where we could prevent multiple lines (or less lines).
          • At a few places you're defining array('class' => ''), this is not required.
          • Use html_writer::link instead of html_writer::tag('a')
          • There are a few places where the html_writer is quite hard to read due to encapsulation of them. It's not blocking, it's my own preference to prevent this.
          • Can you confirm with Barbara that the colours used are OK and not contradictory with other pages?
          • Can you confirm that your new listing elements will nicely expand if there are plenty of options?
          • Can you add some testing instructions related to the listing element itself? Including multiple elements on one page.

          Thanks Jerome!
          Fred

          Show
          Frédéric Massart added a comment - Hi Jerome. Hehe, what a nice new interface we get there, it's nice and smooth! Good job! Here are a few comments: Block Why is by default all hubs are hidden? I'm scared that very few people will notice the 'Show all' button. In non-js mode, the radio button is not aligned with the Hub name. I suppose it makes more sense to enable the downloadable courses by default. Are you sure about using getimagesize() on a remote file? If the purpose of this is to check whether the image exists or not, I'd suggest a HEAD request using a curl object. Can you hardcode the logo to 40x30 because it's a standard? Does this respect the defined proportions on the HUB? #161: Missing period at the end of the comment. #164: I don't like the colon appended to the text, but as you didn't introduce that change let's leave it like that! #171: This was already set before, but as you're editing... the doc_link method is not very extensive and should not be used to append a doc_link to a text, but before. In your case, a white space is used to separate from the text, but that's not a good practice. Not only it does not allow themers to remove it, but switching to RTL might not give the expected result. I'd suggest to manually output the link and use OUTPUT->help_icon() (or other, I haven't checked the code extensively). #176: Why using onclick when you can set the target manually? Though W3C don't like targets as the user should decide. Form element Missing defined('MOODLE_INTERNAL') or die() in the listing element. #113 Missing space between foreach and (). #115 References to the HUB should not be found in this library. #119 Could you comment on the use of  ? In general I prefer avoiding them. #122 Just a semi-colon? #127 IDs have to be unique if you allow for more than one listing element per page. JS We should use the same coding style as in PHP. CamelCase in variable names should not be used. You can maybe make use of the class .jsenabled (set in body) to display your form instead of using JS to switch it. In general, I'm not sure about the use of setHTML(). I'd suggest creating a node and setAttrs() on it. And if you're using setContent(), then do not forget to espace it: Y.Escape.html(). #42: Typo. #46: Non-dynamic selectors should be set a constant to the class so that they're easily identifiable and updatable if need be. #59: Double ; #60: Hard to understand where items is defined. Can you make it clearer with a comment or edit? General In the language file, I'd define a new string and deprecate the other one if need be instead of changing its meaning. There are quite a few lines which are split to early where we could prevent multiple lines (or less lines). At a few places you're defining array('class' => '') , this is not required. Use html_writer::link instead of html_writer::tag('a') There are a few places where the html_writer is quite hard to read due to encapsulation of them. It's not blocking, it's my own preference to prevent this. Can you confirm with Barbara that the colours used are OK and not contradictory with other pages? Can you confirm that your new listing elements will nicely expand if there are plenty of options? Can you add some testing instructions related to the listing element itself? Including multiple elements on one page. Thanks Jerome! Fred
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks Fred.

          Block

          1. in my opinion, only MOOCH contains enough content to be relevant to teachers/students. We'll switch the other tabs on when we have more to display. Having hubs that return 0 results is not great I think. To go further, if people start to use other hubs than MOOCH, there are many more important improvements to do first: the admin should be able to select the default hub, the most searched hub should be the one selected by default when the user reach the page for the first time in his session...
          3. it's what it is at the moment. Can you explain what you mean?
          4. the hubs return 1px image, it's why I test it like that.
          5. we have control of the returned images by MOOCH. Hub server "development" (but should be called maintenance) being driven by MOOCH, other hubs should follow MOOCH standards. In my opinion there should be a fixed ratio for hub image (there is no standard), in order to keep good looking layout. I decided it would this ratio.
          8. in my opinion the info icon could be at the beginning or the end, RTL (Right To Left) or LTF (Left To Right). It is not shocking either way and I highly doubt that any theme designers want to change this anyway. I didn't understand why you said doc_link() should not be used, it seems very good to be used in this case. Can you explain?
          9. I did what you suggest (it frustrates me when my phone opens new tabs) but can't resist to write more: this issue is like 8., some old code. Manual target was till recently not xml strict, W3C decide that it was ok to have target="" again for HTML5. I don't remember if at the time the code was committed in CVS it already changed. W3C is more what you'd call "guidelines" than actual rules.

          JS

          3. can you explain why? I like setHtml(). It's what it's indicated to use to replace content in the YUI doc: http://yuilibrary.com/yui/docs/node/
          5. I use it only once. I would say here I save some memory by not assigning a variable that I use only once.
          7. this is an issue if it's hard to understand (I tried to make it clear in docs but apparently it's not).

          wip

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks Fred. Block 1. in my opinion, only MOOCH contains enough content to be relevant to teachers/students. We'll switch the other tabs on when we have more to display. Having hubs that return 0 results is not great I think. To go further, if people start to use other hubs than MOOCH, there are many more important improvements to do first: the admin should be able to select the default hub, the most searched hub should be the one selected by default when the user reach the page for the first time in his session... 3. it's what it is at the moment. Can you explain what you mean? 4. the hubs return 1px image, it's why I test it like that. 5. we have control of the returned images by MOOCH. Hub server "development" (but should be called maintenance) being driven by MOOCH, other hubs should follow MOOCH standards. In my opinion there should be a fixed ratio for hub image (there is no standard), in order to keep good looking layout. I decided it would this ratio. 8. in my opinion the info icon could be at the beginning or the end, RTL (Right To Left) or LTF (Left To Right). It is not shocking either way and I highly doubt that any theme designers want to change this anyway. I didn't understand why you said doc_link() should not be used, it seems very good to be used in this case. Can you explain? 9. I did what you suggest (it frustrates me when my phone opens new tabs) but can't resist to write more: this issue is like 8., some old code. Manual target was till recently not xml strict, W3C decide that it was ok to have target="" again for HTML5. I don't remember if at the time the code was committed in CVS it already changed. W3C is more what you'd call "guidelines" than actual rules . JS 3. can you explain why? I like setHtml(). It's what it's indicated to use to replace content in the YUI doc: http://yuilibrary.com/yui/docs/node/ 5. I use it only once. I would say here I save some memory by not assigning a variable that I use only once. 7. this is an issue if it's hard to understand (I tried to make it clear in docs but apparently it's not). wip
          Hide
          Frédéric Massart added a comment -

          Hi Jerome,

          I think I got back to you on pretty much everything directly on Jabber. My point #4 on Form was about the entity & nbsp; (Jira ate it).

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Jerome, I think I got back to you on pretty much everything directly on Jabber. My point #4 on Form was about the entity & nbsp; (Jira ate it). Cheers, Fred
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Fred. Sending to integration. I confirm I asked Barbara and the form expands very well with a lof of entries.

          Show
          Jérôme Mouneyrac added a comment - Thanks Fred. Sending to integration. I confirm I asked Barbara and the form expands very well with a lof of entries.
          Hide
          Dan Poltawski added a comment -

          Urm:

          +debugging($url);	
          +error_log(print_r($registeredhub, true));
          
          Show
          Dan Poltawski added a comment - Urm: +debugging($url); +error_log(print_r($registeredhub, true ));
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Resending (I was testing a MOOCH related issue and miss these debugs during my cleaning)

          Show
          Jérôme Mouneyrac added a comment - - edited Resending (I was testing a MOOCH related issue and miss these debugs during my cleaning)
          Hide
          Damyon Wiese added a comment -

          Hi Jerome,

          Thanks for the patch, I like the new search results.

          I found a couple of things that need fixing:

          (major)
          lib/form/listing.php has a relative include
          Shouldn't this be including HTML/QuickForm/input.php instead of button?
          Missing AMOS command to copy 'Download' string.

          (minor)
          Some comments missing fullstops in:
          lib/form/listing.php
          blocks/community/forms.php

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - Hi Jerome, Thanks for the patch, I like the new search results. I found a couple of things that need fixing: (major) lib/form/listing.php has a relative include Shouldn't this be including HTML/QuickForm/input.php instead of button? Missing AMOS command to copy 'Download' string. (minor) Some comments missing fullstops in: lib/form/listing.php blocks/community/forms.php Cheers, Damyon
          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
          Jérôme Mouneyrac added a comment -

          I kept the require_once consistent with all the other forms - I fixed the call as you mentioned. Thanks Damyon.

          Show
          Jérôme Mouneyrac added a comment - I kept the require_once consistent with all the other forms - I fixed the call as you mentioned. Thanks Damyon.
          Hide
          Jérôme Mouneyrac added a comment -

          ah I forgot the AMOS copy, wait...

          Show
          Jérôme Mouneyrac added a comment - ah I forgot the AMOS copy, wait...
          Hide
          Jérôme Mouneyrac added a comment -

          ok done. Is it going to cause an issue if I put the AMOS command into the later commit?

          Show
          Jérôme Mouneyrac added a comment - ok done. Is it going to cause an issue if I put the AMOS command into the later commit?
          Hide
          Damyon Wiese added a comment -

          Hi Jerome, the AMOS commands must be in the same commit that changes the language files - so you'll have to amend your old commit message.

          Show
          Damyon Wiese added a comment - Hi Jerome, the AMOS commands must be in the same commit that changes the language files - so you'll have to amend your old commit message.
          Hide
          Jérôme Mouneyrac added a comment -

          Ah man I'm unlucky today, changing it

          Show
          Jérôme Mouneyrac added a comment - Ah man I'm unlucky today, changing it
          Hide
          Jérôme Mouneyrac added a comment -

          Done, I merged my two last commits

          Show
          Jérôme Mouneyrac added a comment - Done, I merged my two last commits
          Hide
          Damyon Wiese added a comment -

          Sorry Jerome - AMOS commands must each be on their own separate line according to the docs (and that commit message is too long for me to even read properly):

          http://docs.moodle.org/dev/Languages/AMOS#AMOS_script

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Sorry Jerome - AMOS commands must each be on their own separate line according to the docs (and that commit message is too long for me to even read properly): http://docs.moodle.org/dev/Languages/AMOS#AMOS_script Regards, Damyon
          Hide
          Jérôme Mouneyrac added a comment -

          done

          Show
          Jérôme Mouneyrac added a comment - done
          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
          Jérôme Mouneyrac added a comment -

          resubmit

          Show
          Jérôme Mouneyrac added a comment - resubmit
          Hide
          Damyon Wiese added a comment -

          Hi Jerome,

          Not going to push this this week, it just missed the cut off and I don't want to cheat in my first full week integrating. It looks ready to go so will get pushed next week.

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Jerome, Not going to push this this week, it just missed the cut off and I don't want to cheat in my first full week integrating. It looks ready to go so will get pushed next week. Thanks, Damyon
          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.

          Cheers!

          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. Cheers!
          Hide
          Damyon Wiese added a comment -

          Thanks Jerome,

          The AMOS looks fine now.

          This has been pushed to master.

          Show
          Damyon Wiese added a comment - Thanks Jerome, The AMOS looks fine now. This has been pushed to master.
          Hide
          David Monllaó added a comment -

          I've noticed a few things:

          • I can't find the "Install" button, I see a button with the "Download" string
          • The currently selected hub is not highlighted in any way (Linux + Chrome)
          • The "Hide hubs"/"Show all hubs" is a div, so it can not be accessed with the Tab button and there is no way to change the hub with the keyboard

          Waiting for feedback

          Show
          David Monllaó added a comment - I've noticed a few things: I can't find the "Install" button, I see a button with the "Download" string The currently selected hub is not highlighted in any way (Linux + Chrome) The "Hide hubs"/"Show all hubs" is a div, so it can not be accessed with the Tab button and there is no way to change the hub with the keyboard Waiting for feedback
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Right David, the button should be install. Either reject, either create a new issue. Personally I would pass it and create an issue for the Install not being used (my bad I forgot when I changed the string in my last fixes) - one for the hide hubs not very accessible friendly. For the highlight I don't think it's necessary.

          Show
          Jérôme Mouneyrac added a comment - - edited Right David, the button should be install. Either reject, either create a new issue. Personally I would pass it and create an issue for the Install not being used (my bad I forgot when I changed the string in my last fixes) - one for the hide hubs not very accessible friendly. For the highlight I don't think it's necessary.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          After thinking, don't reject it. Pass it. We should not to lose devs/integrators/tester hours for a little string. I created the two new issues. Thank you.

          Show
          Jérôme Mouneyrac added a comment - - edited After thinking, don't reject it. Pass it. We should not to lose devs/integrators/tester hours for a little string. I created the two new issues. Thank you.
          Hide
          David Monllaó added a comment -

          It makes sense, passing it, thanks

          Show
          David Monllaó added a comment - It makes sense, passing it, thanks
          Hide
          Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: