Moodle
  1. Moodle
  2. MDL-31445

moodleforms unique id on field div wrappers

    Details

    • Testing Instructions:
      Hide

      note to tester: you could test the related MDL-31469 at the same time

      For a variety of forms in Moodle (I tested on the quiz settings form, so I would suggest you test on some other complex forms like
      a. add forum,
      b. create multichoice question,
      c. ...)

      1. Go to the form, and make sure it looks/works OK>
      2. Use a tool like firebug to instpect the HTML, and marvel at the beauty of the new id and class attributes on the outer wrapper div for each form field.
      3. Validate the HTML of the page, and make sure there are no problems relating to the ids. (Note that there are other problems, which may include MDL-31469.)

      Show
      note to tester: you could test the related MDL-31469 at the same time For a variety of forms in Moodle (I tested on the quiz settings form, so I would suggest you test on some other complex forms like a. add forum, b. create multichoice question, c. ...) 1. Go to the form, and make sure it looks/works OK> 2. Use a tool like firebug to instpect the HTML, and marvel at the beauty of the new id and class attributes on the outer wrapper div for each form field. 3. Validate the HTML of the page, and make sure there are no problems relating to the ids. (Note that there are other problems, which may include MDL-31469 .)
    • 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:
    • Rank:
      37972

      Description

      It would be useful, when authoring CSS, if MoodleForms gave a unique id to the top level div wrapping each label and field.

        Issue Links

          Activity

          Hide
          Anthony Forth added a comment -

          Look at any MoodleForm and the wrapper for a group or element will now have an id based on it's name.

          Show
          Anthony Forth added a comment - Look at any MoodleForm and the wrapper for a group or element will now have an id based on it's name.
          Hide
          Michael de Raadt added a comment -

          Hi, Anthony.

          Thanks for suggesting that and providing a patch.

          Show
          Michael de Raadt added a comment - Hi, Anthony. Thanks for suggesting that and providing a patch.
          Hide
          Michael de Raadt added a comment -

          Hi, Tim.

          I was wondering if you would like to peer review this patch as you have done related work recently. There's no problem if you don't want to, but I thought I would offer it to you first.

          Michael d;

          Show
          Michael de Raadt added a comment - Hi, Tim. I was wondering if you would like to peer review this patch as you have done related work recently. There's no problem if you don't want to, but I thought I would offer it to you first. Michael d;
          Hide
          Tim Hunt added a comment -

          Yes, I'll take a look later today.

          By the way, this is actually a duplicate of something I started work on a while ago, and never finished: MDL-30204. There is also MDL-10505. Anyway, if Anthony has finally fixed this, it is great. I will make sure there is only one open bug left at the end of today.

          (Also, Michael, in case you don't know, Anthony's desk at the OU is about 5 metres away from mine )

          Show
          Tim Hunt added a comment - Yes, I'll take a look later today. By the way, this is actually a duplicate of something I started work on a while ago, and never finished: MDL-30204 . There is also MDL-10505 . Anyway, if Anthony has finally fixed this, it is great. I will make sure there is only one open bug left at the end of today. (Also, Michael, in case you don't know, Anthony's desk at the OU is about 5 metres away from mine )
          Hide
          Anthony Forth added a comment -

          Tim's and my solutions seem very similar bar I didn't touch the pear lib and, consequently, I am not sure whether Tim's avoids multiple elements with the same id.

          Show
          Anthony Forth added a comment - Tim's and my solutions seem very similar bar I didn't touch the pear lib and, consequently, I am not sure whether Tim's avoids multiple elements with the same id.
          Hide
          Tim Hunt added a comment -

          Review comments:

          1. The code is currently 3 separate commits with bad commit comments. It needs to be rebased down to one commit with a nice comment. (e.g. "MDL-31445 formslib: add a nice id to the wrapper div to help with css"

          2. Apart from that, Anthony's patch is mostly better. Actually, it is remarkable how similar the two patches are!

          3. And one think that is nicer in my patch is that I also add the item type as a class name on the outer div, which can also be helpful for CSS, so I will amend Anthony's commit to deal with 1. and add that bit.

          Show
          Tim Hunt added a comment - Review comments: 1. The code is currently 3 separate commits with bad commit comments. It needs to be rebased down to one commit with a nice comment. (e.g. " MDL-31445 formslib: add a nice id to the wrapper div to help with css" 2. Apart from that, Anthony's patch is mostly better. Actually, it is remarkable how similar the two patches are! 3. And one think that is nicer in my patch is that I also add the item type as a class name on the outer div, which can also be helpful for CSS, so I will amend Anthony's commit to deal with 1. and add that bit.
          Hide
          Anthony Forth added a comment -

          Re. point 1, what should I have done in git to clean this up?

          Show
          Anthony Forth added a comment - Re. point 1, what should I have done in git to clean this up?
          Hide
          Tim Hunt added a comment -

          The magic command is git rebase. Particularly git rebase -i.

          Pro git does not seem to cover it (http://progit.org/book/ch3-6.html) but the "INTERACTIVE MODE" of the man page http://schacon.github.com/git/git-rebase.html describes it pretty well.

          Show
          Tim Hunt added a comment - The magic command is git rebase. Particularly git rebase -i. Pro git does not seem to cover it ( http://progit.org/book/ch3-6.html ) but the "INTERACTIVE MODE" of the man page http://schacon.github.com/git/git-rebase.html describes it pretty well.
          Hide
          Tim Hunt added a comment -

          Anthony, does this amended version look OK? https://github.com/timhunt/moodle/compare/master...MDL-31445

          Show
          Tim Hunt added a comment - Anthony, does this amended version look OK? https://github.com/timhunt/moodle/compare/master...MDL-31445
          Hide
          Tim Hunt added a comment -

          Ah, this does not work, becuase names can contain characters that are illegal in ids. (For example []). We need to add some clean-up.

          Show
          Tim Hunt added a comment - Ah, this does not work, becuase names can contain characters that are illegal in ids. (For example []). We need to add some clean-up.
          Hide
          Tim Hunt added a comment -

          Oh yes. This brings back horrible memories of MDL-30168.

          Show
          Tim Hunt added a comment - Oh yes. This brings back horrible memories of MDL-30168 .
          Hide
          Anthony Forth added a comment -

          Using $element->getAttribute('id') appears to work.

          Show
          Anthony Forth added a comment - Using $element->getAttribute('id') appears to work.
          Hide
          Tim Hunt added a comment -

          OK, I have changed it to use the element id, so now a the outer div for a typical field looks like:

          <div id="fitem_id_timeclose" class="fitem fitem_fdate_time_selector">

          Is that OK. I don't think it is great.

          By the way, I just found MDL-31469 while testing this.

          Show
          Tim Hunt added a comment - OK, I have changed it to use the element id, so now a the outer div for a typical field looks like: <div id="fitem_id_timeclose" class="fitem fitem_fdate_time_selector"> Is that OK. I don't think it is great. By the way, I just found MDL-31469 while testing this.
          Hide
          Anthony Forth added a comment -

          Yes - that's what I was half way through suggesting. Looks good.

          Show
          Anthony Forth added a comment - Yes - that's what I was half way through suggesting. Looks good.
          Hide
          Eloy Lafuente (stronk7) 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.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) 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. TIA and ciao
          Hide
          Aparup Banerjee added a comment -

          Thanks for the nice (team)work here, thats been integrated into master , 22, 21 and is ready for marvelous testing.

          Show
          Aparup Banerjee added a comment - Thanks for the nice (team)work here, thats been integrated into master , 22, 21 and is ready for marvelous testing.
          Hide
          Adrian Greeve added a comment -

          I tested this in version 2.1, 2.2 and master. I did indeed marvel at the beauty of the class ids and attribute names. They look very nice.
          I also ran a validation check on the HTML. Some errors came up mainly in 2.2 and 2.1 about not having a document type declaration, but there were no errors regarding ids and attribute names.
          Test passed.

          Show
          Adrian Greeve added a comment - I tested this in version 2.1, 2.2 and master. I did indeed marvel at the beauty of the class ids and attribute names. They look very nice. I also ran a validation check on the HTML. Some errors came up mainly in 2.2 and 2.1 about not having a document type declaration, but there were no errors regarding ids and attribute names. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers.

          Many thanks & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers. Many thanks & ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: