John, I really like this idea. It's well thought-out and the customization
hooks seem at once powerful and easy to use. I like the use of
<replace-with> to provide a simple way to perform an app-wide appearance
switch. Here are a couple of remarks:
Sorry it's so long. Here is the *tl;dnr* version:
1) The ButtonCell(DefaultAppearance.Resources) constructor can be confusing,
I think it should be dropped.
2) Providing a custom DefaultAppearance.Resources lead to somewhat smelly
code.
3) Minor one-time style modifications are hard to make.
4) Contrary to Stephen, I think this design makes it hard to use
selector-based CSS that assumes the widget obeys a precise DOM structure.
5) We should make sure GIN can be used to inject Appearance
into ButtonCell and Resources into Appearance.
6) I agree with Thomas about:
- Not being sure making Appearance an abstract class is the right choice
- Favoring :hover and other pseudoclasses in the DefaultAppearance.
- - - - -
Now the long version...
*1) The ButtonCell(DefaultAppearance.Resources) constructor can be
confusing, I think it should be dropped.*
This constructor gives the impression that you are using the app-wide
Appearance where in fact you are using DefaultAppearance. This could lead to
the following confusing use case:
- The user has two type of buttons: some initialized with new ButtonCell()
and some with new ButtonCell(otherCellButtonResources)
- He changes the style app-wide via <replace-with>
- Buttons initialized with new ButtonCell() change, the ones initialized
with new ButtonCell(otherCellButtonResources) don't.
Dropping that constructor would make it explicit you are actually forcing
the use of DefaultAppearance.
*2) Providing a custom DefaultAppearance.Resources lead to somewhat smelly
code.*
DefaultAppearance.Resources ties buttonCellStyle() to a specific CSS
@Source. Therefore, I believe overriding it means writing something like:
public interface MyResources extends
ButtonCell.DefaultAppearance.Resources {
@Override
@Source("com/mygroup/myapp/client/ButtonCell.css")
Style buttonCellStyle();
}
Maybe it's just me, but this looks slightly smelly. Also, does GWT.create()
like two @Source for the same method?
*3) Minor one-time style modifications are hard to make.*
Defining a custom Appearance or Resources is good when you want to style all
the buttons in your app, but impractical for small changes like adjusting
padding for a single button. We should think about a way to allow that (and
make sure it works in UiBinder). Ideas:
- Provide an addStyleName() in CellButton and Appearance. I don't like this,
however, as it creates the problem mentioned by Stephen (see point 4 below)
-- moreover, I really like the idea of Appearance moving us to more
"semantic" styling.
- Provide a setProperty(String name, String value) in CellButton and
Appearance and let Appearance apply it however it likes
- Provide semantic methods for frequently applied properties, like
setMargin(), setPadding()...
I'm really not sure I like any of the above proposals however. Maybe we
should just acknowledge that as a limitation of the new styling mechanism?
Is it too limiting?
*4) Contrary to Stephen, I think this design makes it hard to use
selector-based CSS that assumes the widget obeys a precise DOM structure.*
This is because:
- There is no public addStyleName on ButtonCell so the user cannot apply a
custom CSS class and then rely on CSS selectors to style deeper elements.
- The CSS styles are obfuscated (right?), therefore writing a CSS rule that
affects the widget needs an access to DefaultAppearance.Resources.
- DefaultAppearance.Resources is Appearance-specific. So if we modernize the
widget look and feel in a way that changes the widget's DOM structure, we
simply provide a new Resources type. CSS styles developed for the old
DefaultAppearance.Resources cannot be used with the new widget because the
types are not compatible.
*5) We should make sure GIN can be used to inject Appearance
into ButtonCell and Resources into Appearance.*
Weak coupling between ButtonCell, Appearance, and Resource seems like a
perfect use-case for DI. I have not reviewed the code from that perspective,
but it would be nice if we allowed users to subclass ButtonCell and
Appearance to provide @Inject-ed versions.
*6) I agree with Thomas about:*
- Not being sure making Appearance an abstract class is the right choice.
- Favoring :hover and other pseudoclasses in the DefaultAppearance.
Cheers,
Philippe
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
