Support arbitrary relations for stylesheets #18441

okonomiyaki3000 · 2017-10-30T07:49:55Z

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, or preload relations.

Also support other custom relations (but what for?).

brianteeman · 2017-10-30T08:04:17Z

Is this really a good idea. Compiling less files at runtime can be a big performance hit

okonomiyaki3000 · 2017-10-30T08:11:53Z

@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.

brianteeman · 2017-10-30T08:20:33Z

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

okonomiyaki3000 · 2017-10-30T08:25:31Z

Could make it a debug-mode-only feature.

okonomiyaki3000 · 2017-10-30T09:20:23Z

But, ultimately, you can't develop a system that people can't screw up. That probably shouldn't even be a goal.

Fedik · 2017-10-31T21:32:38Z

I am not sure we need such code complication, it already should work fine with:

$doc->addStyleSheet('myLessFile.less', array(), array('relation' => 'stylesheet/less'));

mbabker · 2017-10-31T21:39:21Z

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).

okonomiyaki3000 · 2017-10-31T23:57:27Z

@mbabker There is an issue with that. Because you'd only want to use LESS files during development and switch to compiled css for production. Then your files will be included in a different order in production and development. Which is obviously bad.

mbabker · 2017-11-01T00:06:23Z

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.

okonomiyaki3000 · 2017-11-01T00:15:00Z

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

mbabker · 2017-11-01T00:30:53Z

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.

okonomiyaki3000 · 2017-11-01T00:40:35Z

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'.

mbabker · 2017-11-01T00:49:16Z

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).

okonomiyaki3000 · 2017-11-01T01:00:21Z

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.

mbabker · 2017-11-01T01:19:33Z

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).

okonomiyaki3000 · 2017-11-01T01:24:57Z

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.

C-Lodder · 2017-11-01T13:44:54Z

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

mbabker · 2017-11-01T13:51:07Z

<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">');

C-Lodder · 2017-11-01T14:23:24Z

@mbabker does that also work with JHtml?

mbabker · 2017-11-01T14:26:47Z

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).

mbabker · 2017-11-01T14:32:00Z

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.

…for stylesheets.

okonomiyaki3000 · 2017-11-02T07:19:21Z

I've removed the more controversial part of it and kept only the ability to use custom relations.

okonomiyaki3000 · 2017-12-04T01:55:30Z

Is there any remaining reason not to do this?

mbabker · 2017-12-11T14:33:03Z

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.

denysdesign · 2018-03-19T00:15:33Z

It's not good idea for speed site 😂

okonomiyaki3000 · 2018-03-19T05:37:22Z

@joomla-ua You don't really get it and that's OK. 😂

ghost · 2020-01-12T16:16:56Z

Is there any reason not to close this pull?

okonomiyaki3000 · 2020-01-13T12:01:49Z

@Milglius Yes. The main one being that it's a good and solid improvement and should have been merged ages ago.

HLeithner · 2020-04-07T14:39:43Z

I moved this to 3.10 in my opinion it makes more sense there @zero-24

okonomiyaki3000 · 2020-04-08T14:17:05Z

@HLeithner Great. Merge it.

Fedik · 2020-04-08T18:52:05Z

can then we just make use of rel attribute directly?
$document->addStyleSheet('myLessFile.less', array(), array('rel' => 'stylesheet/less'));
and update renderer for it

okonomiyaki3000 · 2020-04-09T13:00:46Z

@Fedik I believe that already works (the key is relation instead of rel though) but that's only for style sheets. This is for other link tags.

Fedik · 2020-04-09T14:57:04Z

the key is relation instead of rel though

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

zero-24 · 2020-09-25T05:55:22Z

@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

Support the 'stylesheet/less' relation for less files.

849d1f7

Also support other custom relations (but what for?).

joomla-cms-bot added the PR-staging label Oct 30, 2017

Don't specifically support LESS files but do permit custom relations …

b8e7a32

…for stylesheets.

mbabker added this to the Joomla 3.9.0 milestone Dec 11, 2017

okonomiyaki3000 changed the title ~~Support the 'stylesheet/less' relation for less files.~~ Support arbitrary relations for stylesheets Dec 12, 2017

brianteeman modified the milestones: Joomla 3.9.0, Joomla 3.10.0 May 8, 2018

ghost added the J3 Issue label Apr 5, 2019

ghost removed the J3 Issue label Apr 19, 2019

zero-24 modified the milestones: Joomla 3.10.0, Joomla! 3.9.15 Dec 19, 2019

HLeithner modified the milestones: Joomla! 3.9.15, Joomla! 3.9.16 Jan 22, 2020

HLeithner modified the milestones: Joomla! 3.9.16, Joomla! 3.9.17 Mar 4, 2020

HLeithner modified the milestones: Joomla! 3.9.17, Joomla 3.10.0 Apr 7, 2020

zero-24 changed the base branch from staging to 3.10-dev September 25, 2020 05:50

zero-24 removed the PR-staging label Sep 25, 2020

Merge branch '3.10-dev' into support-less-files

7e17563

joomla-cms-bot added the PR-3.10-dev label Sep 25, 2020

Update HeadRenderer.php

e347e7e

zero-24 merged commit af156d2 into joomla:3.10-dev Sep 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support arbitrary relations for stylesheets #18441

Support arbitrary relations for stylesheets #18441

okonomiyaki3000 commented Oct 30, 2017 •

edited

brianteeman commented Oct 30, 2017

okonomiyaki3000 commented Oct 30, 2017

brianteeman commented Oct 30, 2017

okonomiyaki3000 commented Oct 30, 2017

okonomiyaki3000 commented Oct 30, 2017

Fedik commented Oct 31, 2017

mbabker commented Oct 31, 2017 •

edited

okonomiyaki3000 commented Oct 31, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 1, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 1, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 1, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 1, 2017

C-Lodder commented Nov 1, 2017

mbabker commented Nov 1, 2017

C-Lodder commented Nov 1, 2017

mbabker commented Nov 1, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 2, 2017

okonomiyaki3000 commented Dec 4, 2017

mbabker commented Dec 11, 2017

denysdesign commented Mar 19, 2018

okonomiyaki3000 commented Mar 19, 2018

ghost commented Jan 12, 2020

okonomiyaki3000 commented Jan 13, 2020

HLeithner commented Apr 7, 2020

okonomiyaki3000 commented Apr 8, 2020

Fedik commented Apr 8, 2020

okonomiyaki3000 commented Apr 9, 2020

Fedik commented Apr 9, 2020 •

edited

zero-24 commented Sep 25, 2020

Support arbitrary relations for stylesheets #18441

Support arbitrary relations for stylesheets #18441

Conversation

okonomiyaki3000 commented Oct 30, 2017 • edited

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

brianteeman commented Oct 30, 2017

okonomiyaki3000 commented Oct 30, 2017

brianteeman commented Oct 30, 2017

okonomiyaki3000 commented Oct 30, 2017

okonomiyaki3000 commented Oct 30, 2017

Fedik commented Oct 31, 2017

mbabker commented Oct 31, 2017 • edited

okonomiyaki3000 commented Oct 31, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 1, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 1, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 1, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 1, 2017

C-Lodder commented Nov 1, 2017

mbabker commented Nov 1, 2017

C-Lodder commented Nov 1, 2017

mbabker commented Nov 1, 2017

mbabker commented Nov 1, 2017

okonomiyaki3000 commented Nov 2, 2017

okonomiyaki3000 commented Dec 4, 2017

mbabker commented Dec 11, 2017

denysdesign commented Mar 19, 2018

okonomiyaki3000 commented Mar 19, 2018

ghost commented Jan 12, 2020

okonomiyaki3000 commented Jan 13, 2020

HLeithner commented Apr 7, 2020

okonomiyaki3000 commented Apr 8, 2020

Fedik commented Apr 8, 2020

okonomiyaki3000 commented Apr 9, 2020

Fedik commented Apr 9, 2020 • edited

zero-24 commented Sep 25, 2020

okonomiyaki3000 commented Oct 30, 2017 •

edited

mbabker commented Oct 31, 2017 •

edited

Fedik commented Apr 9, 2020 •

edited