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

yui_combo.php doesn't handle shiftered css

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: JavaScript
    • Labels:
    • Testing Instructions:
      Hide

      We have very few core YUI modules with their own CSS in Moodle, so you'll have to fake some first:

      Prep

      • Open lib/yui/src/tooltip/meta/tooltip.json
      • add "moodle-core-tooltip-skin" to the list of requires
      • create the directory structure lib/yui/src/tooltip/assets/skins/sam/
      • Add some recognisable CSS content to a file in that directory named moodle-core-tooltip.css
      • run Shifter on the module/whole of Moodle

      Now to test

      • Navigate to the Login page
      • Take a look at the list of stylesheets in the network panel of your dev toolkit
      • look for an attempt to load a skin for moodle-core-tooltip
      • view that files content
        • Confirm that your recognisable content was found

      And just to make sure:

      Show
      We have very few core YUI modules with their own CSS in Moodle, so you'll have to fake some first: Prep Open lib/yui/src/tooltip/meta/tooltip.json add "moodle-core-tooltip-skin" to the list of requires create the directory structure lib/yui/src/tooltip/assets/skins/sam/ Add some recognisable CSS content to a file in that directory named moodle-core-tooltip.css run Shifter on the module/whole of Moodle Now to test Navigate to the Login page Take a look at the list of stylesheets in the network panel of your dev toolkit look for an attempt to load a skin for moodle-core-tooltip view that files content Confirm that your recognisable content was found And just to make sure: Open http://tangerine.local/moodle/theme/yui_combo.php?moodle/-1/core/tooltip/tooltip.css (where tangerine.local/moodle is the path to your testing moodle installation) confirm that your content is shown
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      I just discovered whilst testing MDL-38666 that we don't properly handle shifted CSS assets.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment - - edited

            Hi Petr, Sam,

            Any chance of a Peer Review on this issue? It's block another issue I have in the integration queue (Converting calendar to use Shifter).

            Cheers,

            A

            Show
            dobedobedoh Andrew Nicols added a comment - - edited Hi Petr, Sam, Any chance of a Peer Review on this issue? It's block another issue I have in the integration queue (Converting calendar to use Shifter). Cheers, A
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            ping

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - ping
            Hide
            phalacee Jason Fowler added a comment -

            [Y] Syntax
            [-] Output
            [Y] Whitespace
            [-] Language
            [-] Databases
            [Y] Testing
            [-] Security
            [-] Documentation
            [Y] Git
            [Y] Sanity check

            Show
            phalacee Jason Fowler added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Andrew D has just pointed out an issue whereby we're not removing -min in the CSS when not debugging.

            There are two options to fix:

            • preg_replace('/-(min|debug)\./', '.', $filename)
            • don't try to use -min|debug in CSS filenames - This would require modifying the config function for Moodle in YUI_config (fairly trivial).

            I personally prefer the latter because if we comboload multiple CSS files, then we'll get more onto the path before we hit the length limit. Possibly a premature optimisation though.

            Show
            dobedobedoh Andrew Nicols added a comment - Andrew D has just pointed out an issue whereby we're not removing -min in the CSS when not debugging. There are two options to fix: preg_replace('/-(min|debug)\./', '.', $filename) don't try to use -min|debug in CSS filenames - This would require modifying the config function for Moodle in YUI_config (fairly trivial). I personally prefer the latter because if we comboload multiple CSS files, then we'll get more onto the path before we hit the length limit. Possibly a premature optimisation though.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Added a fix for the latter option in https://github.com/andrewnicols/moodle/tree/MDL-38667-m-2

            Show
            dobedobedoh Andrew Nicols added a comment - Added a fix for the latter option in https://github.com/andrewnicols/moodle/tree/MDL-38667-m-2
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sorry - to confirm, when jsrev > 0, we currently try to use a -min.css version but shifter doesn't (yet) minify CSS.

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry - to confirm, when jsrev > 0, we currently try to use a -min.css version but shifter doesn't (yet) minify CSS.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Extra commit taking rid of the "-min" for css files... always. Added.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Extra commit taking rid of the "-min" for css files... always. Added.
            Hide
            salvetore Michael de Raadt added a comment -

            There were a few problems with the testing steps.

            The directory structure required (apparently) was /lib/yui/tooltip/assets/skins/sam and the file required as tooltip.css. Don't ask me why, I just followed the errors.

            The following request was in the list...

            http://michael.moodle.local/master_integration/theme/yui_combo.php?moodle/1365146489/core/tooltip/tooltip.css

            ...and contained my CSS content.

            In the file...

            http://michael.moodle.local/master_integration/theme/yui_combo.php?moodle/-1/core/tooltip/tooltip.js

            ...there was some references to 'moodle-core-tooltip', but my CSS was not there.

            To be honest, I really don't know what I was testing or if it has worked at all.

            Show
            salvetore Michael de Raadt added a comment - There were a few problems with the testing steps. The directory structure required (apparently) was /lib/yui/tooltip/assets/skins/sam and the file required as tooltip.css. Don't ask me why, I just followed the errors. The following request was in the list... http://michael.moodle.local/master_integration/theme/yui_combo.php?moodle/1365146489/core/tooltip/tooltip.css ...and contained my CSS content. In the file... http://michael.moodle.local/master_integration/theme/yui_combo.php?moodle/-1/core/tooltip/tooltip.js ...there was some references to 'moodle-core-tooltip', but my CSS was not there. To be honest, I really don't know what I was testing or if it has worked at all.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sorry Michael,

            I had some stupid errors in the testing instructions - Shifter was running in the wrong place, and I'd copied/pasted the wrong URL when I was writing the test instructions.

            I've corrected the instructions and run through them again.

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry Michael, I had some stupid errors in the testing instructions - Shifter was running in the wrong place, and I'd copied/pasted the wrong URL when I was writing the test instructions. I've corrected the instructions and run through them again. Andrew
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Success!

            The content appears correctly now. Thanks for the additional explanations.

            Show
            salvetore Michael de Raadt added a comment - Test result: Success! The content appears correctly now. Thanks for the additional explanations.
            Hide
            poltawski Dan Poltawski added a comment -

            Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

            line 1289 of \lib\changes.php: call to debugging()
            line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
            line 202 of \lib\now.php: call to moodleform->_is_poor_form()
            line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

            Show
            poltawski Dan Poltawski added a comment - Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking. line 1289 of \lib\changes.php: call to debugging() line 281 of \lib\are.php: call to moodleform->detectMissingThanks() line 202 of \lib\now.php: call to moodleform->_is_poor_form() line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13