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

BlockHelper::includeStylesheets $media argument #131

Closed
dbu opened this issue Mar 7, 2014 · 4 comments
Closed

BlockHelper::includeStylesheets $media argument #131

dbu opened this issue Mar 7, 2014 · 4 comments

Comments

@dbu
Copy link
Contributor

dbu commented Mar 7, 2014

I know this is not something that will be easy to fix, but what is the point of the $media argument on the stylesheets call? would it not be the job of the block service to know what stylesheet is for which media? how would the template including the css know? and how would it handle different stylesheets for print and screen?

that said, we tend to define all css in a common place and combine it all with assetic, rather than include fragments on a par page + per block basis, as this is not caching friendly. still as the feature is there, rather have it meaningful.

@rande
Copy link
Member

rande commented Mar 7, 2014

The getStyleSheets and getJavascripts methods are not the best thing in this bundle. However it works with some drawbacks.

The template helpers usage is not mandatory at all. So you can use the helpers and this will not work with assetic or you can create your js/css package with assetic or bower.

Not sure how to respond to you ;)

Can I close this issue ?

@dbu
Copy link
Contributor Author

dbu commented Mar 7, 2014

maybe adding a phpdoc saying @param string $media The css media type to use: all|screen|...

@rande
Copy link
Member

rande commented Mar 7, 2014

Do you want to send a PR ?

@dbu
Copy link
Contributor Author

dbu commented Mar 7, 2014

yeah that is the right answer i guess :-)
just sent the PR

@rande rande closed this as completed Mar 7, 2014
rande added a commit that referenced this issue Mar 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants