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

Already on GitHub? Sign in to your account

Cgridview: rowCssClassExpression and cssClassExpression have a small bug #1163

Closed
smertnik77 opened this Issue Aug 7, 2012 · 11 comments

Comments

Projects
None yet
5 participants

example:
cssClassExpression=>'true?"":"x"'
or
cssClassExpression=>'true?false:"x"'
makes the html code
<td class>

I mean, even a blank value will be added to the class

@ghost ghost assigned cebe Aug 7, 2012

cssClassExpression is from CgridColumn of course)

cebe added a commit to cebe/yii that referenced this issue Aug 8, 2012

Owner

cebe commented Aug 8, 2012

problem also existed in CGridView now also fixed in CGridColumn.

Contributor

rawtaz commented Aug 8, 2012

I'm not sure this is a problem/bug in the first place.

@smertnik77, where are you looking when you conclude that the result is <td class></td>? I am guessing you looked in Chrome's developer tools or something like that, and not in the actual HTML source code that is sent to the browser.

Looking at the diffs from @cebe's commits, my impression is that what was previously output isn't <td class></td> but rather <td class=""></td>, and IMO that is valid HTML. It's just a class attribute that doesn't have any classes in it.

I also tested this now and I do get the class="" for the following code:

<?php

$dataProvider=new CArrayDataProvider(array(
    array('id'=>'Foo1', 'bar'=>'Bar1'),
    array('id'=>'Foo2', 'bar'=>'Bar2'),
));

$this->widget('zii.widgets.grid.CGridView', array(
    'dataProvider'=>$dataProvider,
    'columns'=>array(
        'id',
        array(
            'name'=>'bar',
            'cssClassExpression'=>'true?false:"x"',
        ),
)));

Yii::app()->clientScript->registerCss('foo', '
.yada
{
    background-color: red;
}
');

If on the other hand people prefer not to have the class output when it's empty, I think that's fine. But it's hardly a bug.

Member

mdomba commented Aug 8, 2012

I agree with @rawtaz the output is all standard as the quotes are there, so this "fix" is not really needed but if it's requested we can add it.

Owner

cebe commented Aug 8, 2012

So do we want to "fix" it? @mdomba @samdark @qiangxue ?

One other case that can occur is <tr class="myclass "> with a whitespace inside attribute value, but I think it is still valid html then.

Owner

cebe commented Aug 8, 2012

@smertnik77 was your bug report about the missing ="" or about an empty class attribute?

Member

mdomba commented Aug 8, 2012

IMO it would be more elegant to have clean HTML code so I'm for adding this simple fix.

Owner

cebe commented Aug 8, 2012

Thats also how I see it, vote for it. @rawtaz changed label from Bug -> Enhancement ;-)

Contributor

creocoder commented Aug 8, 2012

Agree with @mdomba about having clean HTML code, so good micro-enhancement.

Contributor

rawtaz commented Aug 8, 2012

Good @cebe. We should do this with many more issues marked as bugs, i.e. make them features ;)

@cebe - about an empty class attribute, I know english bad, sorry

@cebe cebe closed this in 7428985 Aug 10, 2012

samdark added a commit that referenced this issue Aug 10, 2012

Merge pull request #1164 from cebe/1163-gridview-no-empty-class
[ci skip] fixes #1163 CGridview does not create empty class attribute anymore

cebe added a commit that referenced this issue Aug 14, 2012

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