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

Need conditional comments for assets #2906

Closed
exromany opened this issue Mar 28, 2014 · 36 comments
Closed

Need conditional comments for assets #2906

exromany opened this issue Mar 28, 2014 · 36 comments

Comments

@exromany
Copy link

@exromany exromany commented Mar 28, 2014

Need an options in AssetBundle for generating something like this

<!--[if lt IE 9]>
    <link rel="stylesheet" href="css/grid-old-ie.css">
<![endif]-->
<!--[if gt IE 8]><!-->
    <link rel="stylesheet" href="css/grid.css">
<!--<![endif]-->
@samdark
Copy link
Member

@samdark samdark commented Mar 28, 2014

Also you can use body classes instead of embedding scripts in conditions:

<!--[if lt IE 7]> <body class="ie6"> <![endif]--> 
<!--[if IE 7]> <body class="ie7"> <![endif]--> 
<!--[if IE 8]> <body class="ie8"> <![endif]--> 
<!--[if !IE]>--> <body> <!--<![endif]-->
@samdark
Copy link
Member

@samdark samdark commented Mar 28, 2014

Still need it?

@exromany
Copy link
Author

@exromany exromany commented Mar 28, 2014

yep, it still convenient way to include html5shim and others to older browsers

@samdark
Copy link
Member

@samdark samdark commented Mar 28, 2014

  1. How would you like it to be specified?
  2. How to resolve dependencies of normal bundle A on ie8 bundle B?
  3. How to resolve dependencies of ie8 bundle A on normal bundle B?
  4. Can bundle include scripts for both ie6, ie7 and normal ones?
@exromany
Copy link
Author

@exromany exromany commented Mar 28, 2014

how about specify condition per file

public $js = [
    'main.js',
    'html5shiv.js' => 'lt IE 9',
    'respond.js' => 'lt IE 9',
];

or

public $js = [
    'main.js',
    'lt IE 9' => [
        'html5shiv.js',
        'respond.js',
    ],
];
@cebe cebe added the type:feature label Mar 28, 2014
@samdark
Copy link
Member

@samdark samdark commented Mar 28, 2014

OK, what about dependencies?

@exromany
Copy link
Author

@exromany exromany commented Mar 28, 2014

i think that dependency resolution should remain unchanged

@samdark
Copy link
Member

@samdark samdark commented Mar 28, 2014

OK so you assume each dependency will be rendered in HTML in its own separate conditional comment like the following?

<link rel="stylesheet" href="css/grid.css">
<!--[if lt IE 8]>
    <link rel="stylesheet" href="css/grid-ie.css">
<!--<![endif]-->
<!--[if eq IE 9]><!-->
    <link rel="stylesheet" href="css/grid-ie8.css">
<!--<![endif]-->
<!--[if lt IE 8]>
    <link rel="stylesheet" href="css/all-ie.css">
<!--<![endif]-->
@exromany
Copy link
Author

@exromany exromany commented Mar 28, 2014

yep, i think so

@samdark
Copy link
Member

@samdark samdark commented Mar 28, 2014

OK, the only question left is compression and combining assets. How to handle dependencies in this case?

i.e. assets with different conditions could not be combined + order matters.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Mar 28, 2014

I don't think we should support it.

Such code are best directly written in the main layout file rather than being registered through asset bundles.

@samdark
Copy link
Member

@samdark samdark commented Mar 28, 2014

Probably but the issue pops up fairly often and the only obstacle currently is combining assets.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Mar 28, 2014

I don't like introducing a new syntax that is designed to support a feature which is being deprecated.

If this is really of such a great importance and the alternative solution I described is absolutely unacceptable, we should introduce special options in AssetBundle::jsOptions rather than "contaminating" the format of AssetBundle::js. We may then enhance Html::jsFile() to render these special options as conditional comments. This means you should not mix different conditional comments in the same asset bundle, which prevents the asset combining issue.

@egorpromo
Copy link
Contributor

@egorpromo egorpromo commented Mar 29, 2014

I don't like introducing a new syntax that is designed to support a feature which is being deprecated.

+1

@qiangxue qiangxue added this to the 2.0 RC milestone Apr 16, 2014
@yiqing-95
Copy link

@yiqing-95 yiqing-95 commented Apr 26, 2014

if this issue have been settled ? i meet it today and want to know how you guys deal with it .

@exromany
Copy link
Author

@exromany exromany commented Apr 26, 2014

In my project, I do this:

  1. extend yii\web\View::registerJsFile method to wrap script in conditional commnets
  2. in AssetBundle specify 'condition' jsOption

https://gist.github.com/exromany/11320865

@qiangxue qiangxue closed this in 4fcd500 Apr 27, 2014
@qiangxue
Copy link
Member

@qiangxue qiangxue commented Apr 27, 2014

@exromany Thank you for your code. I have made some changes and fixed this issue.

@LordKino
Copy link

@LordKino LordKino commented Oct 28, 2014

this code does not work:
public $js = [
'main.js',
'html5shiv.js' => 'lt IE 9',
'respond.js' => 'lt IE 9',
];

use this code produces an error - the argument can not be an array
public $js = [
'main.js',
'lt IE 9' => [
'html5shiv.js',
'respond.js',
],
]

@LordKino
Copy link

@LordKino LordKino commented Oct 28, 2014

Clearly, thanks!
Another question: how to specify different jquery version for different versions of browsers?

@samdark
Copy link
Member

@samdark samdark commented Oct 28, 2014

You should use separate asset bundles for these.

@LordKino
Copy link

@LordKino LordKino commented Oct 29, 2014

Could you give an example?

@samdark
Copy link
Member

@samdark samdark commented Oct 29, 2014

  1. Create IE6JqueryAsset with corresponding conditional comments.
  2. Create IE7JqueryAsset with corresponding conditional comments.
  3. Via config modify original JqueryAsset. Include dependencies on IE6JqueryAsset and IE7JqueryAsset.
@LordKino
Copy link

@LordKino LordKino commented Oct 30, 2014

As in the third paragraph to indicate the condition does ie8 or another?

@samdark
Copy link
Member

@samdark samdark commented Oct 30, 2014

No. Conditions are set per bundle i.e. in 1 and 2. In 3 you only specifying these as dependencies.

@LordKino
Copy link

@LordKino LordKino commented Oct 30, 2014

How to register in IEJqueryAsset conditional comment?

@samdark
Copy link
Member

@samdark samdark commented Oct 30, 2014

namespace common\components;
use Yii;

class IEJqueryAsset extends yii\web\AssetBundle
{
    public $ sourcePath = null;
    public $ js = [
        '//code.jquery.com/jquery-1.11.0.min.js',
    ];
    public $jsOptions = ['condition' => 'lte IE9'];  // <-------- this!
}
@LordKino
Copy link

@LordKino LordKino commented Oct 30, 2014

'assetManager' => [
    'bundles' => [
        'yii\web\JqueryAsset' =>  [
            'depends' => [
                'common\components\IE_lt_9_JqueryAsset',
            ]
        ],
    ],
],
namespace common\components;
use Yii;
class IE_lt_9_JqueryAsset extends yii\web\AssetBundle
{
    public $ sourcePath = null;
    public $ js = [
        '//code.jquery.com/jquery-1.11.0.min.js',
    ];
    public $cssOptions = ['condition' => 'lt IE 9'];
}

Firefox source code:

........................
<script src="//code.jquery.com/jquery-1.11.0.min.js"></script>
<script src="/assets/19b4aafe/jquery.js"></script>
.........................

@samdark
Copy link
Member

@samdark samdark commented Oct 30, 2014

jsOptions, not cssOptions ;)

@LordKino
Copy link

@LordKino LordKino commented Oct 30, 2014

Did not find this option in the documentation, thank you!

@sushil8901
Copy link

@sushil8901 sushil8901 commented Jan 5, 2016

This option is not working for me. When I open my page in IE8 still jquery 2.1 is loading and not jquery 1.11

my code:

'yii\web\JqueryAsset' => [
     'depends' => [
         'app\assets\IE8Assets',
    ]
],

IE8Assets.php:

class IE8Assets extends AssetBundle
{
    public $basePath = '@webroot';
    public $baseUrl = '@web';
    public $css=[
            'css/ie8.css'
    ];
    public $cssOptions = ['condition' => 'lte IE9'];
    public $js =[
            'js/jquery-1.11.3.min.js',
            'js/jquery-migrate-1.2.1.min.js',
    ]; 
    public $jsOptions = ['condition' => 'lte IE9'];
}
@samdark
Copy link
Member

@samdark samdark commented Jan 5, 2016

Have you added another conditional comment for non-IE assets?

@sushil8901
Copy link

@sushil8901 sushil8901 commented Jan 5, 2016

Nope I dont know how to do that.

@samdark
Copy link
Member

@samdark samdark commented Jan 5, 2016

Same way but specifying !IE as condition. Note that since IE 10 conditional comments aren't supported.

@sushil8901
Copy link

@sushil8901 sushil8901 commented Jan 6, 2016

Thanks @samdark for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.