search

tesis-dynaware / graph-editor

Eclipse Public License 1.0
132 stars 42 forks source link

Moved Grid stroke-color to GraphEditorGrid with styleable property. #9

Closed eckig closed 9 years ago

eckig commented 9 years ago

I left the grid spacing out of the refactoring on purpose because currently it is strongly tied to the GraphEditorProperties. I tested the CSS styleable property with a single GraphEditor view and a GraphEditorContainer.

rmfisher commented 9 years ago

Unfortunately I cannot merge this because it use an internal class that is not part of the JavaFX public API (PaintConverter). As a rule we are not allowed to depend on such classes, certainly not for a change of this kind.

If it's possible to remove the dependency, great, otherwise I think you will have to use your own fork.

eckig commented 9 years ago

That is possible. Should I provide another pull request? Otherwise you can replace PaintConverter.getInstance() yourself with StyleConverter.getPaintConverter().

rmfisher commented 9 years ago

Yeah please provide a new pull request.

rmfisher commented 9 years ago

I refactored the class slightly to meet some of our code formatting requirements. I also changed the style-class to "graph-editor-grid" and the selector to "-grid-color" for consistency. Let me know if there are any problems.

eckig commented 9 years ago

Well this these are just names, no breaking changes. Just curious: Why did you remove the bean and name of the JavaFX properties?

rmfisher commented 9 years ago

As far as I could tell they weren't used. Since we have a rule that all strings have to be defined at the top of the class as constants, I just removed them because it would have been extra clutter.

eckig commented 9 years ago

They are currently not used. But that is the purpose of the properties: Offer developers additional hooks, in this case for changes. Adding the bean and name for properties makes property listeners more reusable, I explained that here: https://eckig.github.io/blog/2014/09/01/using-javafxs-properties/

But in the end it is not a dealbreaker ;-)