Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

can't control position of external links relative to the pipeline #2470

Closed
mjmaax opened this issue Apr 22, 2019 · 4 comments
Closed

can't control position of external links relative to the pipeline #2470

mjmaax opened this issue Apr 22, 2019 · 4 comments

Comments

@mjmaax
Copy link

mjmaax commented Apr 22, 2019

It is not possible to set the position of external links before the pipeline CSS file. I use Grav 1.6.6.

assets:
css_pipeline: true
css_pipeline_include_externals: false

{% do assets.addCss('https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css', { position: before }) %}
{% do assets.addCss('theme://css/style1.css') %}
{% do assets.addCss('theme://css/style2.css') %}

=> the external link (bootstrap.min.css) will appear after the pipeline.

@rhukster
Copy link
Member

Fixed with above commit combined with a slight change in your syntax:

{% do assets.addCss('https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css', { position: 'before' }) %}

Notice the quotes around the string before

rhukster added a commit that referenced this issue Apr 22, 2019
@mjmaax
Copy link
Author

mjmaax commented Apr 23, 2019

Thank you for the fix!

I think the following would be easier to read and also slightly more efficient:

    protected function filterAssets($assets, $key, $value, $sort = false)
    {
        $results = array_filter($assets, function($asset) use ($key, $value) {
            if ($asset[$key] !== $value) return false;
            if ($key === 'position' && $value === 'pipeline' && $asset->getRemote()) {
                $type = $asset->getType();
                if ($this->{$type . '_pipeline_include_externals'} === false) {
                    if ($this->{$type . '_pipeline_before_excludes'}) {
                        $asset->setPosition('after');
                    } else {
                        $asset->setPosition('before');
                    }
                    return false;
                }
            }
            return true;
        });
        if ($sort && !empty($results)) {
            $results = $this->sortAssets($results);
        }
        return $results;
    }

NOTES:

  1. It is currently not possible to override *_pipeline_include_externals with a manual position passed when adding an asset.
  2. I was wondering about the choice of having 2 attributes (_pipeline_include_externals, _pipeline_before_excludes') to control externals. There could be a single '_externals_default_position' attribute which has its default value set to 'pipeline'. We could change it to 'before' or 'after', and possibly override the value when adding a specific asset.

@rhukster
Copy link
Member

you can override the position of an external if you provide a position in the addAsset call. This takes precedence over the system level configurations. The system level configuration is mostly for backward compatibility as prior to 1.6 you couldn't control the position per-item. The go-forward approach is to specify before, pipeline or after when adding the asset. The default is already pipeline, so specifying before or after effectively takes them out of the pipeline.

@mjmaax
Copy link
Author

mjmaax commented Apr 24, 2019

Sorry, what I meant is that it is not possible to specify that by default all externals are out of the pipeline and override one external asset to force it being in the pipeline (by providing position: 'pipeline' in the addAsset call). I have no grav installation to test it at the moment, that is just my guess looking at the code.

By first testing if ($asset[$key] !== $value) we can often avoid several conditions that deal with the specific case of externals, that's why I added it first in the suggested code of my previous message.
Additionally this also save us from testing again that $asset['position'] === 'pipeline' since we know that that this condition is always true.
I also put $asset->getRemote() at its correct place for clarity/efficiency.
The code is equivalent, but I believe slightly more efficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants