Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32744

Topics format CSS selector is hardcoded in course dragdrop.js

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course, JavaScript
    • Labels:
      None
    • Testing Instructions:
      Hide

      Make sure that dragdrop works correctly for existing formats. If the new contributed format is added, drag-drop should not work unless it has format.js file with at least M.course.format.get_config function exist and contains configuration.

      Show
      Make sure that dragdrop works correctly for existing formats. If the new contributed format is added, drag-drop should not work unless it has format.js file with at least M.course.format.get_config function exist and contains configuration.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32744-master-3

      Description

      This will cause that for other format types drag-drop may work incorrectly

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            kabalin Ruslan Kabalin added a comment -

            Added patch, further re-factoring of MDL-32657

            Show
            kabalin Ruslan Kabalin added a comment - Added patch, further re-factoring of MDL-32657
            Hide
            kabalin Ruslan Kabalin added a comment -

            I am afraid, this is ready to revision.

            Show
            kabalin Ruslan Kabalin added a comment - I am afraid, this is ready to revision.
            Hide
            kabalin Ruslan Kabalin added a comment - - edited

            For example how this work with moodle-format_topcoll external format, see: https://github.com/kabalin/moodle-format_topcoll/commit/985dd91714c526bf1ea93dd6aa562898dcb67333 (should be used along with the patch in this issue)

            Show
            kabalin Ruslan Kabalin added a comment - - edited For example how this work with moodle-format_topcoll external format, see: https://github.com/kabalin/moodle-format_topcoll/commit/985dd91714c526bf1ea93dd6aa562898dcb67333 (should be used along with the patch in this issue)
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear Ruslan,

            Just got your message, thank you - going to try the patch now.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Dear Ruslan, Just got your message, thank you - going to try the patch now. Cheers, Gareth
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear Ruslan,

            Thank you so much for you hard work . I have integrated the patch into 'https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32744' as the closing 'tbody' was in the wrong place, the strings needed a little refactoring, slight coding issue with 'document.getElementById("sectioncontent-"+theToggle);' to 'document.getElementById("sectionbody-"+theToggle);' in 'module.js', the $displaysection variable needed fixing with the removal of 'course_get_display' etc. - but I'm not completely convinced that the code works properly yet for this functionality.

            When moving the sections the text within the 'tr.cps td' does not move consistently - any ideas why? My thought is an additional swap in 'format.js'.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Dear Ruslan, Thank you so much for you hard work . I have integrated the patch into 'https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32744' as the closing 'tbody' was in the wrong place, the strings needed a little refactoring, slight coding issue with 'document.getElementById("sectioncontent-"+theToggle);' to 'document.getElementById("sectionbody-"+theToggle);' in 'module.js', the $displaysection variable needed fixing with the removal of 'course_get_display' etc. - but I'm not completely convinced that the code works properly yet for this functionality. When moving the sections the text within the 'tr.cps td' does not move consistently - any ideas why? My thought is an additional swap in 'format.js'. Cheers, Gareth
            Hide
            kabalin Ruslan Kabalin added a comment -

            Regarding fixes, I have just demonstrated you the general idea, I did not investigate how your format works, so there must be plenty of inconsistencies I was not aware about (the obvious one is language strings for different subformats). I leave it to you to make it work properly

            Show
            kabalin Ruslan Kabalin added a comment - Regarding fixes, I have just demonstrated you the general idea, I did not investigate how your format works, so there must be plenty of inconsistencies I was not aware about (the obvious one is language strings for different subformats). I leave it to you to make it work properly
            Hide
            gb2048 Gareth J Barnard added a comment -

            To be honest Ruslan, you were fairly spot on the first time. I just need to understand the mechanisms implemented in 'format.js'

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - To be honest Ruslan, you were fairly spot on the first time. I just need to understand the mechanisms implemented in 'format.js' Cheers, Gareth
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear Ruslan,

            I see on GitHub you have forked my Master branch. I also have a MDL-32744 branch for this very issue which is worth using instead so that changes can be swapped between the two for this.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Dear Ruslan, I see on GitHub you have forked my Master branch. I also have a MDL-32744 branch for this very issue which is worth using instead so that changes can be swapped between the two for this. Cheers, Gareth
            Hide
            poltawski Dan Poltawski added a comment -

            Hi,

            I really wanted to send this straight for integration for you today. But I couldn't because:

            1. It conflcits with master: spinners, left/right stuff and I didn't know if I was removing the right things
            2. It doesn't have testing instructions.

            So sorry, but I couldn't do that.

            Show
            poltawski Dan Poltawski added a comment - Hi, I really wanted to send this straight for integration for you today. But I couldn't because: It conflcits with master: spinners, left/right stuff and I didn't know if I was removing the right things It doesn't have testing instructions. So sorry, but I couldn't do that.
            Hide
            kabalin Ruslan Kabalin added a comment -

            Thanks a lot Dan, my fault. I have re-based the patch and added testing instruction.

            Show
            kabalin Ruslan Kabalin added a comment - Thanks a lot Dan, my fault. I have re-based the patch and added testing instruction.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Gents,

            This code is proving to be such a nightmare that Buffy the Vampire Slayer could not deal with, so I've created CONTRIB-3652 to help me get to grips with all of the changes that need to be implemented for Moodle 2.3 - any comments appreciated.

            But pragmatically, I do consider restricting tags to 'ul' and 'li' to be bad because of creativity with course formats (and indeed possibly the concept of course layouts, my comments on MDL-33027) which is why this code is so important.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Gents, This code is proving to be such a nightmare that Buffy the Vampire Slayer could not deal with, so I've created CONTRIB-3652 to help me get to grips with all of the changes that need to be implemented for Moodle 2.3 - any comments appreciated. But pragmatically, I do consider restricting tags to 'ul' and 'li' to be bad because of creativity with course formats (and indeed possibly the concept of course layouts, my comments on MDL-33027 ) which is why this code is so important. Cheers, Gareth
            Hide
            poltawski Dan Poltawski added a comment -

            git pull git://git.luns.net.uk/moodle.git MDL-32744-master-2
            remote: Counting objects: 29, done.
            remote: Compressing objects: 100% (12/12), done.
            remote: Total 15 (delta 9), reused 0 (delta 0)
            Unpacking objects: 100% (15/15), done.
            From git://git.luns.net.uk/moodle
             * branch            MDL-32744-master-2 -> FETCH_HEAD
            Auto-merging course/yui/toolboxes/toolboxes.js
            Auto-merging course/format/weeks/format.js
            Auto-merging course/format/topics/format.js
            CONFLICT (content): Merge conflict in course/format/topics/format.js
            Automatic merge failed; fix conflicts and then commit the result.

            Show
            poltawski Dan Poltawski added a comment - git pull git://git.luns.net.uk/moodle.git MDL-32744-master-2 remote: Counting objects: 29, done. remote: Compressing objects: 100% (12/12), done. remote: Total 15 (delta 9), reused 0 (delta 0) Unpacking objects: 100% (15/15), done. From git://git.luns.net.uk/moodle * branch MDL-32744-master-2 -> FETCH_HEAD Auto-merging course/yui/toolboxes/toolboxes.js Auto-merging course/format/weeks/format.js Auto-merging course/format/topics/format.js CONFLICT (content): Merge conflict in course/format/topics/format.js Automatic merge failed; fix conflicts and then commit the result.
            Hide
            kabalin Ruslan Kabalin added a comment -

            Grrrr

            Show
            kabalin Ruslan Kabalin added a comment - Grrrr
            Hide
            kabalin Ruslan Kabalin added a comment -

            merging issue is fixed

            Show
            kabalin Ruslan Kabalin added a comment - merging issue is fixed
            Hide
            poltawski Dan Poltawski added a comment -

            I have integrated and tested this.

            This really came too late and had hiccups on the way but I integrated it for the benefit of our contrib course format authors despite these hurdles.

            Show
            poltawski Dan Poltawski added a comment - I have integrated and tested this. This really came too late and had hiccups on the way but I integrated it for the benefit of our contrib course format authors despite these hurdles.
            Hide
            kabalin Ruslan Kabalin added a comment -

            Thank you Dan.

            Show
            kabalin Ruslan Kabalin added a comment - Thank you Dan.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

            Thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12