allow the usage of 'name' and 'filter' in an CLinkColumn #970

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
4 participants
@thyseus

thyseus commented Jul 13, 2012

allow the usage of 'name' and 'filter' in an CLinkColumn

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Jul 14, 2012

Owner

It will allow custom names and filtering for link columns. Right?

Need changelog line.

Owner

samdark commented Jul 14, 2012

It will allow custom names and filtering for link columns. Right?

Need changelog line.

thyseus added some commits Jul 13, 2012

enhancements to CLinkColumn
allow usage of 'name', 'sortable' and 'filter' in CLinkColumn
enhancements to CLinkColumn
Merge branch 'master' of https://github.com/thyseus/yii

Conflicts:
	framework/zii/widgets/grid/CLinkColumn.php
@thyseus

This comment has been minimized.

Show comment Hide comment
@thyseus

thyseus Jul 14, 2012

exact, it makes sorting, filtering possible when a 'name' attribute is specified.

it is demanded from some people in the forums:

http://www.yiiframework.com/forum/index.php/topic/7175-clinkcolumn-and-sorting/

with this solution, its more elegant. can be merged in one commit, thank you.

thyseus commented Jul 14, 2012

exact, it makes sorting, filtering possible when a 'name' attribute is specified.

it is demanded from some people in the forums:

http://www.yiiframework.com/forum/index.php/topic/7175-clinkcolumn-and-sorting/

with this solution, its more elegant. can be merged in one commit, thank you.

@cebe

View changes

framework/zii/widgets/grid/CLinkColumn.php
* @see labelExpression
*/
public $label='Link';
+

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Jul 14, 2012

Owner

missing newlines between properties is convention and should stay as it is.

@cebe

cebe Jul 14, 2012

Owner

missing newlines between properties is convention and should stay as it is.

@ghost ghost assigned mdomba Jul 14, 2012

@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Jul 14, 2012

Owner

btw: while all this code is copy and paste from CDataColumn, wouldn't it be easier/better to extend from CDataColumn instead?
Only thing to handle then is that $value property has to get ignored/does not have to be mandatory.

Owner

cebe commented Jul 14, 2012

btw: while all this code is copy and paste from CDataColumn, wouldn't it be easier/better to extend from CDataColumn instead?
Only thing to handle then is that $value property has to get ignored/does not have to be mandatory.

easier implementation of CLinkColumn Enhancements
- like cebe said, we just need to extend from CDataColumn. Excellent
  idea and very OOP-ish!
- removed newlines
- for me everything works (sorting, filtering) - please test
@thyseus

This comment has been minimized.

Show comment Hide comment
@thyseus

thyseus Jul 15, 2012

the newlines are removed to keep convention.

cebe's wonderful idea to extend from CDataColumn worked ! (thyseus/yii@2863d5e)

i tried to rebase the 4 commits onto 8fe4a4 ( Merge pull request #969 from suralc/ht...) which worked
for my local repository, but git does not let me push to github. still a git newbie, sorry for the mess.

thyseus commented Jul 15, 2012

the newlines are removed to keep convention.

cebe's wonderful idea to extend from CDataColumn worked ! (thyseus/yii@2863d5e)

i tried to rebase the 4 commits onto 8fe4a4 ( Merge pull request #969 from suralc/ht...) which worked
for my local repository, but git does not let me push to github. still a git newbie, sorry for the mess.

thyseus added some commits Jul 15, 2012

CLinkColumn Enhancements
- allows the usage of 'name' in CLinkColumns
- like cebe said, we just need to extend from CDataColumn. Excellent
  idea and very OOP-ish!
- for me everything works (sorting, filtering) - please test
removed 'filter' from CLinkColumn
which is not needed anymore because of extending from CDataColumn
@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Jul 15, 2012

Owner

@thyseus do not rebase, as it changes history, always use merge.

Owner

cebe commented Jul 15, 2012

@thyseus do not rebase, as it changes history, always use merge.

@thyseus

This comment has been minimized.

Show comment Hide comment
@thyseus

thyseus Jul 22, 2012

@mdomba : since the best solution only changes a few lines, i can remove this pull request and create a clean one for merging. including @SInCE. agreed?

thyseus commented Jul 22, 2012

@mdomba : since the best solution only changes a few lines, i can remove this pull request and create a clean one for merging. including @SInCE. agreed?

@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Jul 22, 2012

Owner

@thyseus before creating a new pull request, we should make clear what to do with the $value property, which has to be ignored/does not have to be mandatory in the Link column. We need to handle this in init() method, but I am still not sure what would be the best way to go here.

Owner

cebe commented Jul 22, 2012

@thyseus before creating a new pull request, we should make clear what to do with the $value property, which has to be ignored/does not have to be mandatory in the Link column. We need to handle this in init() method, but I am still not sure what would be the best way to go here.

@thyseus

This comment has been minimized.

Show comment Hide comment
@thyseus

thyseus Jul 22, 2012

Agreed. How about:

public function init()
{
    // name and value are not mandatory, since CLinkColumn accepts the 'label' property
    if($this->name===null)
        $this->sortable=false;
    return true;
}

thyseus commented Jul 22, 2012

Agreed. How about:

public function init()
{
    // name and value are not mandatory, since CLinkColumn accepts the 'label' property
    if($this->name===null)
        $this->sortable=false;
    return true;
}
@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Jul 23, 2012

Owner

CDataColumn does not do anything in init() so this should work, but I am not sure if we should rely on this by not calling parent::init();. Any opinions @qiangxue @samdark @mdomba ?

Btw: return true; is not necessary here.

Owner

cebe commented Jul 23, 2012

CDataColumn does not do anything in init() so this should work, but I am not sure if we should rely on this by not calling parent::init();. Any opinions @qiangxue @samdark @mdomba ?

Btw: return true; is not necessary here.

@thyseus

This comment has been minimized.

Show comment Hide comment
@thyseus

thyseus Jul 23, 2012

calling parent::init() would make value or name mandatory, which is not what we want.

i will remove the return true.

thyseus commented Jul 23, 2012

calling parent::init() would make value or name mandatory, which is not what we want.

i will remove the return true.

@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Jul 23, 2012

Owner

calling parent::init() would make value or name mandatory, which is not what we want.

We could work around that by setting $value to a non null value, call parent::init() and reset it to null afterwards.
Not a good solution but would work too and one can be sure init() is called when implementing it. These are the two cases we have to ponder about.

Owner

cebe commented Jul 23, 2012

calling parent::init() would make value or name mandatory, which is not what we want.

We could work around that by setting $value to a non null value, call parent::init() and reset it to null afterwards.
Not a good solution but would work too and one can be sure init() is called when implementing it. These are the two cases we have to ponder about.

@mdomba

This comment has been minimized.

Show comment Hide comment
@mdomba

mdomba Jul 26, 2012

Member

@cebe adding a value to call init and then removing it is a hack and does not look good in the core.

I'm not sure if this change is needed at all, what is the problem of using

    array(
      'name' => 'field_name',
      'type' => 'raw',
      'value' => 'CHtml::link($data->field_name,$data->field_name)'
    ),
Member

mdomba commented Jul 26, 2012

@cebe adding a value to call init and then removing it is a hack and does not look good in the core.

I'm not sure if this change is needed at all, what is the problem of using

    array(
      'name' => 'field_name',
      'type' => 'raw',
      'value' => 'CHtml::link($data->field_name,$data->field_name)'
    ),
@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Jul 26, 2012

Owner

@mdomba thats the reason why I am not finallly happy ;)

will check if you solution does all whats needed.

Owner

cebe commented Jul 26, 2012

@mdomba thats the reason why I am not finallly happy ;)

will check if you solution does all whats needed.

@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Jul 27, 2012

Owner

@mdomba yep, verified. This would do it. So it is not necessary to add this functionality to CLinkColumn.
@thyseus do you have any use case that does not work with mdombas solution?

Owner

cebe commented Jul 27, 2012

@mdomba yep, verified. This would do it. So it is not necessary to add this functionality to CLinkColumn.
@thyseus do you have any use case that does not work with mdombas solution?

@thyseus

This comment has been minimized.

Show comment Hide comment
@thyseus

thyseus Jul 29, 2012

then we could remove CLinkColumn completely. Its just a matter of style, i like using a CLinkColumn for links. i am ok if it does not go into the core though ;)

thyseus commented Jul 29, 2012

then we could remove CLinkColumn completely. Its just a matter of style, i like using a CLinkColumn for links. i am ok if it does not go into the core though ;)

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Aug 1, 2012

Owner

In case of CLinkColumn, labelExpression is very close to value from CDataColumn. What if we'll extend from CDataColumn and set value with labelExpression?

Owner

samdark commented Aug 1, 2012

In case of CLinkColumn, labelExpression is very close to value from CDataColumn. What if we'll extend from CDataColumn and set value with labelExpression?

@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Aug 2, 2012

Owner

@samdark Sounds like a good solution :)

Owner

cebe commented Aug 2, 2012

@samdark Sounds like a good solution :)

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Sep 8, 2012

Owner

@thyseus do you have time to work on this more?

Owner

samdark commented Sep 8, 2012

@thyseus do you have time to work on this more?

@thyseus

This comment has been minimized.

Show comment Hide comment
@thyseus

thyseus Sep 12, 2012

Yes, i have time. I just am not sure what to do about this.

In my opinion extending from CDataColumn works perfectly.

Of course, this is not necessary, but it makes CLinkColumn a lot more useful.

I vote for samdark's implementation. Its really just about changing
"extends CGridColumn" with "extends CDataColumn".

thyseus commented Sep 12, 2012

Yes, i have time. I just am not sure what to do about this.

In my opinion extending from CDataColumn works perfectly.

Of course, this is not necessary, but it makes CLinkColumn a lot more useful.

I vote for samdark's implementation. Its really just about changing
"extends CGridColumn" with "extends CDataColumn".

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Sep 12, 2012

Owner

Well, go with my solution then. The main workload here seems to be testing for backwards-compatibility breaks and errors.

Owner

samdark commented Sep 12, 2012

Well, go with my solution then. The main workload here seems to be testing for backwards-compatibility breaks and errors.

@mdomba

This comment has been minimized.

Show comment Hide comment
@mdomba

mdomba Oct 18, 2012

Member

As commented above the CLinkColumn is not needed anymore so there is no need to "complicate" it further as that would be just a duplication of code or "hacks" to solve issues... so it's decided to deprecate it for the next release...

Member

mdomba commented Oct 18, 2012

As commented above the CLinkColumn is not needed anymore so there is no need to "complicate" it further as that would be just a duplication of code or "hacks" to solve issues... so it's decided to deprecate it for the next release...

@samdark samdark closed this Oct 18, 2012

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Oct 18, 2012

Owner

CLinkColumn will be there for simple usage only and will not be deprecated. If you need more options, consider using value.

Owner

samdark commented Oct 18, 2012

CLinkColumn will be there for simple usage only and will not be deprecated. If you need more options, consider using value.

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