[Trinidad] Patches! Fresh patches! Patches anybody?

[my...@eekboom.com]

What is the best way to contribute to Trinidad?
I went ahead and supplied two patches for issues I was having with missing skinning features: TRINIDAD-755, TRINIDAD-745

Right now, I am missing another feature (putting labels _above_ fields).
I am a little hesitant to supply yet another patch while I haven't heard anything on my older patches.

Can a committer please have a look at my previous patches and comment on them? I am willing to put some more work into them if you see any flaws, but it would be great if in the end the features would make it into the code base.

Thanks a lot!

Re: [Trinidad] Patches! Fresh patches! Patches anybody?

[Stephen Friedrich <my...@eekboom.com>]

Thanks for your answer, Adam.
I'll try to be more careful in keeping formatting and not changing unrelated
code. I am just so used to act on Idea's inspection warning's to keep the code
status green.

I'll provide some separate patches to clean up the code a little:
Unused imports, missorted modifiers, maybe some other un-controversial stuff.
Have I got a "go" for that?

Is there a defined code style that I can use to configure my IDE's formatting
and inspections?

I tried to discuss the icon position issue, but admittedly I already started
coding before:
http://www.nabble.com/%22required%22-icon-after-label%3A-How-to-best-implement-skinning--tf4551820.html
There wasn't much feedback (Thanks, Simon!), but maybe that was because because
I forgot to add the "[Trinidad]" prefix.
I'll remember that, plus I'll choose a more general thread title and be more
insistent on feedback the next time.

Please see the patches as base for discussion. Main reason I wrote the code first
is that I had no idea beforehand how long it would take me to understand and
change the code. I wouldn't want to promise anything I couldn't deliver later.

I am afraid I do not really understand your comment about "af|outputLabel".
First I haven't found any other usages of the icons besides in outputLabel.
Then, all skinning properties in SkinProperties.java include the component
they apply to as a prefix. (The skin selector does in fact only mention the
new skinning properties for the ouputLabel component.)
I'll study the code some more.

Adam Winer wrote:
> Hey, Stephen, a couple of comments:
> - The patch for 755 includes a lot of changes that aren't specific to
>   your work (removing unnecessary imports, whitespace adjustments, etc.)
>   If you want to create separate, minor issues of "Unnecessary imports",
>   and attach a separate patch there, that's cool.
> - It's really good to have some discussions over the APIs, instead of
>   asking for patches to be checked in as is.
> - Looking at the patch, it seems as though you're using properties
>   including "af|outputLabel";  yet the skin selector doc just referred to
>   -tr-required-icon-position.  I think this is a skinning property that is
>   not at all specific to outputLabel, so the doc is right, the code wrong.
> 
> -- Adam
> 
> 
> On 10/12/07, myfaces-dev@eekboom.com <my...@eekboom.com> wrote:
>> What is the best way to contribute to Trinidad?
>> I went ahead and supplied two patches for issues I was having with missing skinning features: TRINIDAD-755, TRINIDAD-745
>>
>> Right now, I am missing another feature (putting labels _above_ fields).
>> I am a little hesitant to supply yet another patch while I haven't heard anything on my older patches.
>>
>> Can a committer please have a look at my previous patches and comment on them? I am willing to put some more work into them if you see any flaws, but it would be great if in the end the features would make it into the code base.
>>
>> Thanks a lot!
>>

Re: [Trinidad] Patches! Fresh patches! Patches anybody?

[Adam Winer <aw...@gmail.com>]

Hey, Stephen, a couple of comments:
- The patch for 755 includes a lot of changes that aren't specific to
  your work (removing unnecessary imports, whitespace adjustments, etc.)
  If you want to create separate, minor issues of "Unnecessary imports",
  and attach a separate patch there, that's cool.
- It's really good to have some discussions over the APIs, instead of
  asking for patches to be checked in as is.
- Looking at the patch, it seems as though you're using properties
  including "af|outputLabel";  yet the skin selector doc just referred to
  -tr-required-icon-position.  I think this is a skinning property that is
  not at all specific to outputLabel, so the doc is right, the code wrong.

-- Adam


On 10/12/07, myfaces-dev@eekboom.com <my...@eekboom.com> wrote:
> What is the best way to contribute to Trinidad?
> I went ahead and supplied two patches for issues I was having with missing skinning features: TRINIDAD-755, TRINIDAD-745
>
> Right now, I am missing another feature (putting labels _above_ fields).
> I am a little hesitant to supply yet another patch while I haven't heard anything on my older patches.
>
> Can a committer please have a look at my previous patches and comment on them? I am willing to put some more work into them if you see any flaws, but it would be great if in the end the features would make it into the code base.
>
> Thanks a lot!
>