Uploaded image for project: '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:

      Description

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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            anthonyforth 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
            anthonyforth 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
            salvetore Michael de Raadt added a comment -

            Hi, Anthony.

            Thanks for suggesting that and providing a patch.

            Show
            salvetore Michael de Raadt added a comment - Hi, Anthony. Thanks for suggesting that and providing a patch.
            Hide
            salvetore 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
            salvetore 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
            timhunt 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
            timhunt 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
            anthonyforth 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
            anthonyforth 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
            timhunt 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
            timhunt 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
            anthonyforth Anthony Forth added a comment -

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

            Show
            anthonyforth Anthony Forth added a comment - Re. point 1, what should I have done in git to clean this up?
            Hide
            timhunt 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
            timhunt 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
            timhunt Tim Hunt added a comment -

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

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

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

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

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

            Show
            anthonyforth Anthony Forth added a comment - Using $element->getAttribute('id') appears to work.
            Hide
            timhunt 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
            timhunt 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
            anthonyforth Anthony Forth added a comment -

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

            Show
            anthonyforth Anthony Forth added a comment - Yes - that's what I was half way through suggesting. Looks good.
            Hide
            stronk7 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
            stronk7 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
            nebgor 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
            nebgor 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
            abgreeve 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
            abgreeve 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  12/Mar/12