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

          Attachments

            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