WIP: Additional stylable properties support for ImageView #30
Conversation
| @Override | ||
| public Boolean convert(ParsedValue<String, Boolean> value, Font font) { | ||
| final String sval = value != null ? value.getValue() : null; | ||
| return "true".equalsIgnoreCase(sval); |
There was a problem hiding this comment.
I think we define "true" as a const
There was a problem hiding this comment.
Since 9 there is a public API javafx.css.converter.BooleanConverter that should be used.
| SizeConverter.getInstance(), 0.0) { | ||
|
|
||
| @Override | ||
| public boolean isSettable(ImageView n) { |
There was a problem hiding this comment.
general question: should we define params in new code as final?
There was a problem hiding this comment.
FYI this code was C&P from Node thus it follows the same code semantics and structure as previously existing code. Changing ImageView would require updating all other classes that follow the same programming idioms.
There was a problem hiding this comment.
sure, that's why I said 'general question' ;) Do not make sense to do it here. Just something that came in my mind when I saw your PR.
| @Override | ||
| public Boolean convert(ParsedValue<String, Boolean> value, Font font) { | ||
| final String sval = value != null ? value.getValue() : null; | ||
| return "true".equalsIgnoreCase(sval); |
There was a problem hiding this comment.
Since 9 there is a public API javafx.css.converter.BooleanConverter that should be used.
|
@andres can you target this PR to develop branch (instead of master)? That way, appveyor might pass. |
|
Yes, I’ll rebase the PR with |
|
Will we need an update on the css reference document as well? |
|
@svenreimers yes, absolutely. |
|
Is this PR waiting for an update on the css reference document? |
|
No, this issue never got to the point where it is ready for code review. There are API design issues that would need to be hammered out first, such as whether we should add more styleable properties for nodes such as ImageView; if so, which attributes; how it interacts with layout. |
|
@kevinrushforth should the discussion be moved to the mailing list? |
kevinrushforth
changed the title
kevinrushforth
changed the title
|
As announced in this message, the official This sandbox repository is being retired on 1-Oct-2019. You will need to migrate your WIP pull request to the openjdk/jfx repo if you want to continue working on it. Here are instructions for migrating your pull request. The updated CONTRIBUTING.md doc has additional information on how to proceed. Once you have done this, it would be helpful to add a comment with a pointer to the new PR. NOTE: since this is a feature / enhancement request it needs to be discussed on the openjfx-dev mailing list if you want it to be considered. The new openjdk/jfx repo will be open for pull requests on Wednesday, 2-Oct-2019. I will send email to the openjfx-dev mailing list announcing this. |
Relates to #29.
Intended as a the start of discussion as not all properties (
viewport,x,ystill missing) have been added as stylable.