[#18441] - Support arbitrary relations for stylesheets
- Fixed in Code Base
- 25 Sep 2020
- Medium
- Build: staging
- # 18441
- Diff
- okonomiyaki3000:support-less-files
- Success hound No violations found. Woof! Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
User tests: Successful: Unsuccessful:
Also support other custom relations (but what for?).
Pull Request for Issue # .
Summary of Changes
Check for a relation attribute when creating stylesheet link tags (use stylesheet by default).
If the filename ends in .less, use the stylesheet/less as the relation so that the file can be parsed by less.js (not included here).
Testing Instructions
- Include one or more less files and specify the appropriate relation.
$document = JFactory::getDocument();
$document->addStyleSheet('myLessFile.less', array(), array('relation' => 'stylesheet/less'));- Also you'll need
less.js.
$document = JFactory::getDocument();
$document->addScript('https://cdn.jsdelivr.net/npm/less@2.7.3/dist/less.min.js');Expected result
Your less file should be loaded on the page and compiled with less.js.
Actual result
Documentation Changes Required
Yes, probably.
Note: There are several other good uses of this, including using the
alternate,prefetch, orpreloadrelations.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
@brianteeman I suppose you mean this is not really a good idea. And you're right, in production environment. This should only be used for development where it saves you a bit a trouble when you're making a lot of changes to your less files. In production you should use compiled and minified css, of course.
exactly. but if we add this feature then i can almost guarantee some themes will use it by default all the time and then people will say joomla is slow. My 2c
Could make it a debug-mode-only feature.
But, ultimately, you can't develop a system that people can't screw up. That probably shouldn't even be a goal.
I am not sure we need such code complication, it already should work fine with:
$doc->addStyleSheet('myLessFile.less', array(), array('relation' => 'stylesheet/less'));
Stylesheets have a hardcoded relation.
The HtmlDocument subclass has a addHeadLink() method which this would work just fine for this without a core modification, as long as there are no issues with the LESS files added in that method being in the document before the stylesheets added through addStyleSheet() (the <head> render processes the subclass' _links property before it handles the _styleSheets property from the base Document class).
From everything I can gather rel="stylesheet/less" is not even a valid HTML definition (https://html.spec.whatwg.org/multipage/links.html#linkTypes and https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types) so personally I'd be very cautious about opening the door to allowing the rel attribute of a <link> tag explicitly rendering stylesheets be something other than what the HTML specification allows.
https://www.w3.org/TR/html4/types.html#type-links lists all of the common link-types but also adds that: "Authors may wish to define additional link types not described in this specification." which is exactly what the creators of LESS have done. There's nothing weird going on here. People have been using LESS in this way for years. http://lesscss.org/#client-side-usage
And that can already be accomplished with other existing API methods. Conceivably accepting this PR also means that the stylesheet processing should conceivably handle any file which may render a stylesheet with special conditions.
The only thing that makes this PR worthwhile is the issue you noted of the load order of elements. Beyond that, this PR doesn't add anything useful to the API in my view except a special case for a single file format.
If it's an issue with creating a special case for LESS, that could actually be removed and just leave in support for custom relations instead of hard coding 'stylesheet'.
For me it's not even that. I'm just not seeing what use case is being added to the API that isn't already possible, even if you have to go to a somewhat unconventional load method.
if ($compileLess) {
$this->addHeadLink('templates/' . $this->template . '/css/template.less', 'stylesheet/less');
} else {
$this->addHeadLink('templates/' . $this->template . '/css/template.css', 'stylesheet');
}
// Or if you still want to use `HTMLHelper` to get file paths
if ($compileLess) {
$file = \Joomla\CMS\HTML\HTMLHelper::_('stylesheet', 'template.less', ['pathOnly' => true, 'version' => 'auto', 'relative' => true, 'detectDebug' => false], []);
$relation = 'stylesheet/less';
} else {
$file = \Joomla\CMS\HTML\HTMLHelper::_('stylesheet', 'template.min.css', ['pathOnly' => true, 'version' => 'auto', 'relative' => true, 'detectDebug' => true], []);
$relation = 'stylesheet';
}
$this->addHeadLink($file, $relation);The load order remains consistent, and depending on your preferences your template style definitions would be the first media resource loaded no matter what unless someone else is pushing in media with that trick (since right now component media will get loaded before the template is included).
No. The load order becomes a mess. Your own less/css files will still load in the correct order relative to each other but they'll all load ahead of any other css files (like bootstrap, for example) which you certainly don't want.
Also, ideally, HTMLHelper::includeRelativeFiles would get an update so that when you pass it 'template.less', it uses that in debug mode but uses the compiled and minified css version otherwise. Similar to how it currently tries to use unminified css in debug mode. But that doesn't really need to be part of the core, that's a function anyone could add if he wanted such functionality.
Ya well that one's all going to boil down to template workflow (if you're loading Bootstrap through JHtml::_('bootstrap.loadCss'); as Beez3 can be configured to be or compiling it direct into your template as we do on the joomla.org template).
Honestly the only concrete answer right now is "no, you cannot load LESS through addStyleSheet() in a manner compatible with tools like less.js". Anything else is doable depending on what technical tradeoffs you want to make (ranging from using another method with conditional logic to an onAfterRender event handler changing the output to just overloading the renderer class as some extension providers do).
So, leaving LESS out of it, what would be the possible negative impact of using the 'rel' attribute passed by the user instead of hard coding 'stylesheet' when rendering the stylesheet links? I can see none.
Rather than specifically adding support for rel="stylesheet/less", why not simply allow to user to add their own if specified?
By that, I mean remove this line:
$relation = ($relation == 'stylesheet' && \JFile::getExt($src) == 'less') ? 'stylesheet/less' : 'stylesheet';
and just use
$relation = isset($attribs['relation']) ? $attribs['relation'] : 'stylesheet';
In Joomla 3, we can then (finally!) start using preload
<link rel="preload" href="/styles/other.css" as="style">You already can do preload.
/*
* Note that both of these methods exist in HtmlDocument only, the redheaded stepchild known as ErrorDocument would need other handling as it does for everything
*/
// Before stylesheets are rendered
$this->addHeadLink('/styles/other.css', 'preload', 'rel', ['as' => 'style']);
// After stylesheets are rendered
$this->addCustomTag('<link rel="preload" href="/styles/other.css" as="style">');
Yes and no. Yes in that you can get the proper file path to use for the tag. No in that the JHtml methods won't directly write it for you.
$customCss = HTMLHelper::_('stylesheet', 'custom.css', ['pathOnly' => true, 'version' => 'auto', 'relative' => true, 'detectDebug' => false], []);Passing the pathOnly flag in the options array tells the method you want it to return the file path it resolves instead of adding the file to the stylesheet queue in the document instance (both stylesheet() and script() support this).
With that being said though, I don't know if I would add something to JHtml in 3.x unless it is compatible with the preload support added in 4.0 which is actually a feature added to JDocument (basically a new key in the options array, preload, with its value being an array of the preload relations to use, i.e. 'preload' => array('preload')), this way we aren't immediately adding support for some kind of preload support then deprecating it for another manner of doing the same thing.
| Labels |
Added:
?
|
||
I've removed the more controversial part of it and kept only the ability to use custom relations.
Is there any remaining reason not to do this?
| Milestone |
Added: |
||
Still don't necessarily agree with changing a method to render tags for stylesheets to allow any arbitrary value but apparently I'm alone in my thinking as usual so if it's properly tested out it can be merged as with any other feature proposal.
| Title |
|
||||||
| Title |
|
||||||
It's not good idea for speed site
@joomla-ua You don't really get it and that's OK.
| Milestone |
Removed: |
||
| Milestone |
Added: |
||
Is there any reason not to close this pull?
@HLeithner Great. Merge it.
can then we just make use of rel attribute directly?
$document->addStyleSheet('myLessFile.less', array(), array('rel' => 'stylesheet/less'));
and update renderer for it
the key is
relationinstead ofrelthough
yeah, my point is to keep the same name of attribute, without replacing/renaming,
then it will work as other attributes, and may be rendered as other attributes
but not important
| Labels |
Removed:
?
|
||
| Labels |
Added:
?
|
||
@Fedik @okonomiyaki3000 I have just implemented the change suggested by @Fedik to use rel and will merge this for now. Thanks for the patience on this one @okonomiyaki3000
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-09-25 05:55:29 |
| Closed_By | ⇒ | zero-24 |
Is this really a good idea. Compiling less files at runtime can be a big performance hit