Dear Julian,<div><br></div><div>Thanks for the excellent feedback! It will come in handy if I want to contribute something else in the future. </div><div><br></div><div>No, I wasn't aware that valueBlock could take two arguments and you're right that doing it that way does give more flexibility than what I implemented. So I'd like to withdraw my proposed changes.</div>
<div><br></div><div>Thank you,</div><div><br></div><div>-- Dan<br><br><div class="gmail_quote">On Wed, Oct 6, 2010 at 6:26 PM, Julian Fitzell <span dir="ltr"><<a href="mailto:jfitzell@gmail.com">jfitzell@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Hi Dan,<br>
<br>
Thanks. I took a quick look at this. Are you aware that you can<br>
currently do the following?<br>
<br>
WAReportColumn new<br>
valueBlock: [ :pet :html | html anchor url: pet url; with: pet petName ];<br>
title: 'Name'<br>
<br>
At first glance, this seems to give more flexibility, but if you feel<br>
it doesn't meet your needs, I'm happy to look at it again.<br>
<br>
Since I did review the .mcz before I found that method, though, I'll<br>
pass on the feedback I had noted:<br>
<br>
* We don't use #ifNil:ifNotNil: for portability reasons. use "isNil<br>
ifTrue: [] ifFalse: []"<br>
* There are a couple of methods in your version that have been<br>
changed but have no actual changes in them. Please make sure to check<br>
the changes before committing your version and revert any methods<br>
needed to remove these cases (they lead to conflicts down the road and<br>
confusion about who wrote the method)<br>
* Our coding conventions put spaces around the contents of blocks and<br>
cascaded method sends each on their own lines.<br>
<br>
So this:<br>
<br>
(aColumn anchorBlock) ifNil: [html text: text]<br>
ifNotNil: [html anchor url: (aColumn anchorForRow: aRow); with: text]<br>
<br>
Should really be more like:<br>
<br>
aColumn anchorBlock isNil<br>
ifTrue: [ html text: text ]<br>
ifFalse: [ html anchor<br>
url: (aColumn anchorForRow: aRow);<br>
with: text ]<br>
<br>
None of these are the end of the world (i.e. we can fix them as we<br>
merge) but the easier you can make it for us, the more we'll love you.<br>
:)<br>
<br>
Julian<br>
<div><div></div><div class="h5"><br>
On Tue, Oct 5, 2010 at 2:28 PM, Dan Winkler <<a href="mailto:dan.winkler@gmail.com">dan.winkler@gmail.com</a>> wrote:<br>
> Attached is my enhancement to WATableReport which allows items in the table<br>
> to link to other web pages. Previously it was only possible to attach a<br>
> Smalltalk callback to an item, not a regular HTML anchor to another web<br>
> page.<br>
> Example usage:<br>
><br>
> petsTable := WATableReport new.<br>
> petsTable<br>
> columns:<br>
> ((OrderedCollection new)<br>
> add:<br>
> ((WAReportColumn new)<br>
> valueBlock: [ :pet | pet petName ];<br>
> anchorBlock: [ :pet | pet url ];<br>
> title: 'Name');<br>
> add: (WAReportColumn selector: #age title: 'Age');<br>
> add: (WAReportColumn selector: #birthDate title: 'Birth Date');<br>
> add: (WAReportColumn selector: #gender title: 'Gender');<br>
> add: (WAReportColumn selector: #breed title: 'Breed');<br>
> add: (WAReportColumn selector: #petNumber title: 'Number');<br>
> yourself)<br>
</div></div>> _______________________________________________<br>
> seaside-dev mailing list<br>
> <a href="mailto:seaside-dev@lists.squeakfoundation.org">seaside-dev@lists.squeakfoundation.org</a><br>
> <a href="http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev" target="_blank">http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev</a><br>
><br>
><br>
_______________________________________________<br>
seaside-dev mailing list<br>
<a href="mailto:seaside-dev@lists.squeakfoundation.org">seaside-dev@lists.squeakfoundation.org</a><br>
<a href="http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev" target="_blank">http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev</a><br>
</blockquote></div><br></div>