[
https://issues.apache.org/jira/browse/TRINIDAD-2120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13405393#comment-13405393
]
Prakash Udupa commented on TRINIDAD-2120:
-----------------------------------------
General comments:
=============
Trinidad-2120 could have been used to track discussion to JQuery ThemeRoller
integration. For specific feature code changes, I suggest you report specific
JIRA issues. There is possibly two that I'm seeing from your patch...
1. Enhance -tr-property-ref to be able to include specific segments of property
values in a shorthand property
2. Looks like you are trying to solve another requirement / issue regarding
'ui-icon'.
Review comments for code change in
'TRINIDAD-2120-4-changes-trinidad-impl-only.patch':
===============================================================
I liked the 'strategy' pattern you used, that makes it easy to expand this
feature to other properties that can take shorthand values.
General Documentation:
------------------------
http://myfaces.apache.org/trinidad/devguide/skinning.html
There is a section that describes the -tr-property-ref feature. Please add
notes about the enhancement you did here.
StyleUtils.java:
---------------
You are letting selectorNames ending with 'ui-icon' as not being icons.
- return (selectorName.endsWith("-icon") ||
+ return ( (selectorName.endsWith("-icon") &&
!selectorName.equals("ui-icon") && !selectorName.contains(".ui-icon")) ||
There are no comments around this change on why this is required. From the dev
guide, the documented convention is that all selectors ending in 'icon' are to
be specially treated as icons (they result in icon objects). There could be
legit icons selectors whose names end with 'ui-icon', and those will possibly
break with your change.
IncludePropertyNode.java:
--------------------------------------
public IncludePropertyNode(
String name,
String selector,
String propertyName,
String localPropertyName,
String location)
- There is no javadoc on the 'location' property. Please include an example
when you doc it.
- And then this constructor has duplicate code. The other constructor's body
should be just calling your new one passing a 'null' location.
- location / getLocation is confusing, if I understand this right, it is
specifically the CSS standard name of the property segment inside of a
shorthand property that you are trying to extract. Can you consider giving it a
more intuitive name, for example:
segmentPropertyName / getSegmentPropertyName
SkinStyleSheetParserUtils.java:
----------------------------------------------
private static IncludePropertyNode _createIncludePropertyNode(
String propertyName,
String propertyValue)
{
if (values.length == 3)
{
location = trimQuotes(values[2].trim());
}
...
}
- Remove white space line inside the if statement.
StyleSheetDocument.java:
--------------------------
You handled the use-case #2:
// 2. Resolve included properties
// color: -tr-property-ref(".AFDefaultFont:alias","background-color");
- Looks like you missed to handle the use-case for #2-prime:
// 2'. Resolve included properties in a compact selector.
// e.g., border: 1px solid -tr-property-ref(".AFDarkColor:alias",color);
I see #2-prime is as much a common usecase as is #2
- Make "CSSUtils._isLength(String value)" as a public method, remove the
underscore prefix, and then just use it in LengthExtractor
- Add a similar isBorderStyle() method to CSSUtils and use it in
BorderStyleExtractor.
- You added a public static method trimQuotes() that is a duplicate of the
already existing _trimQuotes(), remove the public method.
- Consider renaming 'COMPOSITE_SELECTORS' to
'COMPOSITE_PROPERTIES_EXTRACTOR_MAP' because that is what it is, consider
adding a code comment briefly explaining this data structure.
- For sake of improved readability, move all the private static extractor
classes towards the end of the file where all the private static classes are,
and also move the static initializer block to after all the static classes.
- Add javadoc style comments to Extractor interface .
// used when resolving -tr-property-ref
// given a list of PropertyNodes to traverse, find the one whose name matches
the propertyName.
// then we assign the value to the localPropertyName.
// .AFFoo:alias {color: red} af|test {background-color:
-tr-property-ref(".AFFoo:alias", "color");
// localPropertyName is background-color
// propertyName is color
// properties is .AFFoo:alias's properties
private List<PropertyNode> _getIncludedProperties(
String propertyName,
String localPropertyName,
Iterable<PropertyNode> properties)
{
- This method is no more needed because you took care of its implementation in
the new method you added that takes the additional 'location' parameter. Please
remove this method, and make sure you carry the nice comments on top of it to
your new method, and add a bit more comments about the additional parameter.
- Fix the indentation for the new code you added into _getIncludedProperties()
to be 2 spaces - it currently is an inconsistent 4 spaces.
CSSUtils.java:
--------------------
- Your newly added CSSUtils.isColor() method is almost a complete code
duplication of parseColor()
Your implementation could instead be just this...
public static boolean isColor(String value)
{
return (parseColor(value) != null);
}
> Use jQuery ThemeRoller skins with Trinidad
> ------------------------------------------
>
> Key: TRINIDAD-2120
> URL: https://issues.apache.org/jira/browse/TRINIDAD-2120
> Project: MyFaces Trinidad
> Issue Type: Improvement
> Components: Skinning
> Affects Versions: 2.0.0-core
> Reporter: Leonardo Uribe
> Assignee: Leonardo Uribe
> Attachments: TRINIDAD-2120-1.patch, TRINIDAD-2120-2.patch,
> TRINIDAD-2120-4-changes-trinidad-impl-only.patch, cupertino+casablanca.png,
> redmond+casablanca-2.png, redmond+casablanca.png,
> screenshot-trinidad-cupertino.PNG, screenshot-trinidad-smoothness.PNG,
> screenshot-trinidad-sunny.PNG, south-street+casablanca-2.png
>
>
> Here is the original mail from Trasca Virgil:
> http://markmail.org/search/?q=themeroller%20trinidad#query:themeroller%20trinidad+page:1+mid:byczdawpyj33zqoy+state:results
> Mon, 25 Oct 2010 07:01:25 -0700
> Hi
>
> I am interested to get better skinning support in Apache MyFaces. I want
> to
> get MyFaces closely integrated with http://jqueryui.com/themeroller/ - I am
> targeting MyFaces JSF1.2 branch.
>
> The end result should be the same with what PrimeFaces already did
> - http://www.primefaces.org/themes.html
>
> My initial idea is to implemented a JQueryCssToMyFacesCss kind of compiler
> which
> will get as input the jquery CSS syntax and will dump MyFaces CSS syntax.
>
> I have few questions related with this:
> * Did anybody tried something similar in the past - in the MyFaces
> community?
> * Do you think the approach is achievable? Do you have a better
> suggestion? Is
> the UI MyFaces CSS syntax a generic enough UI css framework or is making
> MyFaces
> specific assumptions?
> * Is this doable only by implementing the previous compiler or the
> MyFaces/Trinidad components should be touched also?
>
> Here is the documentation for jQuery UI CSS framework
>
> http://docs.jquery.com/UI/Theming/API
>
> Thank you,
> Virgil
> Investigating more about this possible improvement, I notice that jquery
> themeroller themes does not require jquery to work. So what can we do?
> We can take themeroller themes and generate a skin from trinidad. Trinidad
> already has all the pieces of the pluzze (css parser/merger and a cool
> skinning api) so we should just use it.
> I tried to create a skin in this way:
> <skin>
> <id>sunny.desktop</id>
> <family>sunny</family>
> <render-kit-id>org.apache.myfaces.trinidad.desktop</render-kit-id>
>
> <style-sheet-name>skins/themeroller/sunny/jquery-ui-1.8.14.custom.css</style-sheet-name>
> </skin>
> <skin-addition>
> <skin-id>sunny.desktop</skin-id>
>
> <style-sheet-name>skins/themeroller/trinidad-theme.css</style-sheet-name>
> </skin-addition>
> The first stylesheet is the reference to a generated jquery theme and the
> addition is the file that does the integration with trinidad. So, ThemeRoller
> generates the .css + image files and we just need to provide a reusable .css
> file to reuse the css classes. In practice with just one file we can create
> 20 or 30 trinidad themes in one move!
> Obviously these skins are no match for casablanca theme, and will possibly
> have some flaws (the same for any themeroller skin, right?), but I think it
> is worth to try it. I'll attach some files here to show how it looks like.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
