Details

    • Type: Sub-task
    • Status: Closed
    • Priority: 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

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dobedobedoh 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
            dobedobedoh 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
            jerome 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
            jerome 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
            dobedobedoh 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
            dobedobedoh 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            jerome Jérôme Mouneyrac added a comment -

            Fixes done, sending to Fred for peer-review

            Show
            jerome Jérôme Mouneyrac added a comment - Fixes done, sending to Fred for peer-review
            Hide
            fred 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
            fred 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
            jerome 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
            jerome 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
            fred 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
            fred 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
            jerome 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
            jerome 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
            poltawski Dan Poltawski added a comment -

            Urm:

            +debugging($url);	
            +error_log(print_r($registeredhub, true));
            

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

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

            Show
            jerome 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 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 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            jerome 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
            jerome 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
            jerome Jérôme Mouneyrac added a comment -

            ah I forgot the AMOS copy, wait...

            Show
            jerome Jérôme Mouneyrac added a comment - ah I forgot the AMOS copy, wait...
            Hide
            jerome 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
            jerome 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 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 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
            jerome Jérôme Mouneyrac added a comment -

            Ah man I'm unlucky today, changing it

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

            Done, I merged my two last commits

            Show
            jerome Jérôme Mouneyrac added a comment - Done, I merged my two last commits
            Hide
            damyon 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 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
            jerome Jérôme Mouneyrac added a comment -

            done

            Show
            jerome Jérôme Mouneyrac added a comment - done
            Hide
            cibot CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            resubmit

            Show
            jerome Jérôme Mouneyrac added a comment - resubmit
            Hide
            damyon 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 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 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 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 Damyon Wiese added a comment -

            Thanks Jerome,

            The AMOS looks fine now.

            This has been pushed to master.

            Show
            damyon Damyon Wiese added a comment - Thanks Jerome, The AMOS looks fine now. This has been pushed to master.
            Hide
            dmonllao 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
            dmonllao 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            dmonllao David Monllaó added a comment -

            It makes sense, passing it, thanks

            Show
            dmonllao David Monllaó added a comment - It makes sense, passing it, thanks
            Hide
            damyon 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 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:
                  Fix Release Date:
                  14/May/13