Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Asset helper #64

Merged
merged 8 commits into from
Mar 21, 2017
Merged

Asset helper #64

merged 8 commits into from
Mar 21, 2017

Conversation

michalbundyra
Copy link
Member

@michalbundyra michalbundyra commented Jun 12, 2016

Asset view helper. It required resource map (array) configuration. Can be used also with JSON file (eg. generated by gulp/grunt - rev-manifest.json) as described in docs.

I am not 100% happy with tests because of confusing compatibility with zend-servicemanager v2 and v3.

Any suggestions would be appreciated.

@weierophinney
Copy link
Member

This looks like a nice solution in terms of being light-weight, and integrating with established front-end workflows.

My one comment: I wonder if this should integrate with basePath()? As an example, once the path is resolved from the resource map, should that path then be passed to basePath() in order to ensure it is resolved correctly? My inclination is yes, but the question is if the helper should do that internally, or if we should document that the workflow should be:

<?= $this->basePath($this->asset('css/style.css')) ?>

In particular, the use of relative URLs in the resource map may be problematic when on a path that represents more than one level deep in the tree (e.g., '/foo/bar/baz' would make 'css/style.css' resolve to '/foo/bar/css/style.css' on most browsers). If the asset paths are expected to be relative to the application root, we need to either enforce that within the helper, or document that the developer is responsible for this.

Thoughts?

@michalbundyra
Copy link
Member Author

@weierophinney good point. I thought about it, and I think developer should be responsible to ensure he has a good paths. We can add into documentation example with basePath helper, everything depends on resource map array - if there are relative or absolute paths.

I think better if we allow decide developers how they want use it. Probably it will be used mostly with plugins headLink, headScript, ... I can imagine also that maybe somebody will use it as following:

'view_helper_config' => [
    'asset' => [
        'resource_map' => [
            'jquery' => 'https://code.jquery.com/jquery-3.0.0.min.js',
        ],
    ],
],

and in view:

<?= $this->headScript()->appendFile($this->asset('jquery')) ?>

It's hard to say how it will be used, but I think without integration with basePath we have a greater capacity of use.

BTW. How you'd like to use one plugin inside another? Pass it via factory?

which will be replaced by versioned asset name defined in `resource_map`
of the configuration.

### Note
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put the note in a blockqoute.

> ### Note
> ...

return $this->resourceMap[$asset];
}

public function setResourceMap($resourceMap)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a full DocBlock. (description, parameter, return value)

return $this;
}

public function getResourceMap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

/**
* View helper plugin to fetch asset from resource map.
*/
class Asset extends AbstractHelper
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the helper in the DocBlock of PhpRenderer for code completion in IDEs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 3e08baf

@michalbundyra
Copy link
Member Author

@froschdesign I've made all requested changes and added also a few fixes

@weierophinney weierophinney added this to the 2.9.0 milestone Mar 20, 2017
@weierophinney weierophinney merged commit bddbc6b into zendframework:develop Mar 21, 2017
weierophinney added a commit that referenced this pull request Mar 21, 2017
weierophinney added a commit that referenced this pull request Mar 21, 2017
weierophinney added a commit that referenced this pull request Mar 21, 2017
@weierophinney
Copy link
Member

Thanks, @webimpress

@michalbundyra michalbundyra deleted the feature/asset-helper branch October 10, 2019 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants