Joomla! Issue Tracker | Joomla! CMS #12375 - Add version and IE conditional statements to JHtml::stylesheet/JDocument::addStyleSheet methods and deprecate JDocument::addSyleSheetVersion()

? ?

Fixed in Code Base
4 Nov 2016
Medium
Build: staging
# 12375
Diff
andrepereiradasilva:stylesheets

Success

Success continuous-integration/drone the build was successful Details
Success continuous-integration/travis-ci/pr The Travis CI build passed Details
Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details

User tests: Successful: Unsuccessful:

andrepereiradasilva
9 Oct 2016

Pull Request for Improvement.

Summary of Changes

This PR does for stylesheets what was done for script in #11289

Ie, adds the ability to add add media version and IE conditional statements to the stylesheets link tag using JDocument::addStyleSheet() and JHtml::stylesheet() methods.

Also deprecates JDocument::addSyleSheetVersion() method.

Testing Instructions

Please test this in all possible scenarios.
Any problem will get stylesheets not loading.

Do a code review.
Use 3.7.x branch and apply patch
Check backend and frontend styles are working fine
Add external css stylesheets like to isis/protostar index.php, some examples:

// JDocument B/C tests
/* addStyleSheet($url, $type = 'text/css', $media = null, $attribs = array()) */
$this->addStyleSheet('/path/to/external/bcaddStyleSheet1.css');
$this->addStyleSheet('/path/to/external/bcaddStyleSheet2.css', '', null, array());
$this->addStyleSheet('/path/to/external/bcaddStyleSheet3.css', 'text/css', 'all', array('scriptid' => 1));
/* addStyleSheetVersion($url, $version = null, $type = 'text/css', $media = null, $attribs = array()) */
$this->addStyleSheetVersion('/path/to/external/bcaddStyleSheetVersion1.css');
$this->addStyleSheetVersion('/path/to/external/bcaddStyleSheetVersion2.css', null, '', null, array());
$this->addStyleSheetVersion('/path/to/external/bcaddStyleSheetVersion3.css', 'version-string', 'text/css', 'all', array('scriptid' => 1));

// JDocument new method signature tests
/* addStyleSheet($url, $options = array(), $attribs = array()) */
$this->addStyleSheet('/path/to/external/addStyleSheet0.css');
$this->addStyleSheet('/path/to/external/addStyleSheet1.css', array(), array('id' => 'stylesheetid'));
$this->addStyleSheet('/path/to/external/addStyleSheet2.css', array(), array('id' => 'stylesheetid', 'data-test' => 1));
$this->addStyleSheet('/path/to/external/addStyleSheet3.css', array(), array('data-attrib' => 'original-value'));
$this->addStyleSheet('/path/to/external/addStyleSheet4.css', array(), array('data-attrib' => 'final-value'));
$this->addStyleSheet('/path/to/external/addStyleSheet5.css', array(), array('data-attrib' => array('item1' => 'value1', 'item2' => 'value2')));
$this->addStyleSheet('/path/to/external/addStyleSheet6.css', array('version' => 'auto'), array('data-attrib' => array('item1' => 'value1', 'item2' => 'value2')));
$this->addStyleSheet('/path/to/external/addStyleSheet7.css', array('version' => 'version-string'), array('media' => 'all', 'data-attrib' => array('item1' => 'value1', 'item2' => 'value2')));
$this->addStyleSheet('/path/to/external/addStyleSheet8.css', array('version' => 'auto', 'conditional' => 'lf IE 9'), array('data-attrib' => array('item1' => 'value1', 'item2' => 'value2')));

// JHTML B/C Tests
/* stylesheet($file, $attribs = array(), $relative = false, $path_only = false, $detect_browser = true, $detect_debug = true) */
JHtml::_('stylesheet', 'media/system/css/jquery.Jcrop.min.css');
JHtml::_('stylesheet', 'system/frontediting.css', array('id' => 'stylesheetid'), true, false, false, false);

// JHtml new method signature tests
/* stylesheet($file, $options = array(), $attribs = array()) */
JHtml::_('stylesheet', 'media/com_finder/css/indexer.css', array(), array('data-attrib' => array('item1' => 3, 'item2' => 'value2')));
JHtml::_('stylesheet', 'system/adminlist.css', array('relative' => true), array('data-attrib' => array('item1' => 3, 'item2' => 'value2')));
JHtml::_('stylesheet', 'system/calendar-jos.css', array('relative' => true, 'version' => 'auto'), array('data-attrib' => array('item1' => 3, 'item2' => 'value2')));
JHtml::_('stylesheet', 'system/mootree.css', array('relative' => true, 'version' => 'version-string'), array('media' => 'all', 'data-attrib' => array('item1' => 3, 'item2' => 'value2')));
JHtml::_('stylesheet', 'system/mootree_rtl.css', array('relative' => true, 'version' => 'version-string', 'conditional' => 'lf IE 9'), array('media' => 'all', 'data-attrib' => array('item1' => 3, 'item2' => 'value2')));

Check the HTML output and confirm all is fine
Do the same tests for anything else you can remember ### Observations

From a B/C perspective i think there are no issues, but mantainers please check.

156b01c 9 Oct 2016

let's do the same for stylesheets

andrepereiradasilva - open - 9 Oct 2016

andrepereiradasilva - change - 9 Oct 2016

Status

New

⇒

Pending

joomla-cms-bot - change - 9 Oct 2016

Category

Libraries

⇒

Libraries Unit Tests

73fc98a 31 Oct 2016

Merge branch 'staging' into stylesheets

Fedik - test_item - 31 Oct 2016 - Tested successfully

Fedik - comment - 31 Oct 2016

I have tested this item ✅ successfully on 73fc98a

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12375.}

Fedik - comment - 31 Oct 2016

only thing I have noticed, that some IDE interpret addStyleSheet and addScript as deprecated, that really can confuse people ?

andrepereiradasilva - comment - 31 Oct 2016

only thing I have noticed, that some IDE interpret addStyleSheet and addScript as deprecated, that really can confuse people ?

i can change it to note on both after this is merged

dgt41 - comment - 31 Oct 2016

@andrepereiradasilva @Fedik actually this is good, devs should use ONLY JHtml

Fedik - comment - 31 Oct 2016

well, it seems correct way
http://softwareengineering.stackexchange.com/questions/229644/how-to-annotate-method-signature-changes

so all fine here ?

andrepereiradasilva - comment - 2 Nov 2016

ok following #11289 and #12373 all js files are now referenced with media version in the core.

We need to do this in css files also! (particuly with 3.7.0 isis redesign)

One more test here please so this can be merged and after make a PR to add media version to all css files in the core.

zero-24 - change - 2 Nov 2016

Milestone

Added:

brianteeman - comment - 3 Nov 2016

Tested successfully

no sha again @mbabker

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12375.}

mbabker - comment - 3 Nov 2016

Whomever keeps breaking my app, please stop.

brianteeman - comment - 3 Nov 2016

I have tested this item ✅ successfully on 73fc98a

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12375.}

brianteeman - test_item - 3 Nov 2016 - Tested successfully

andrepereiradasilva - comment - 3 Nov 2016

thanks @brianteeman !

andrepereiradasilva - comment - 3 Nov 2016

@jeckodevelopment RTC this one too?

jeckodevelopment - change - 3 Nov 2016

Status	Pending	⇒	Ready to Commit
Labels

jeckodevelopment - comment - 3 Nov 2016

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12375.}