Add Transitions with :indefinite examples - #31
Conversation
vlaaad
left a comment
vlaaad
left a comment
There was a problem hiding this comment.
Good stuff! There are some minor issues I want to see adressed first, then I'd be happy to merge it.
| :on-cycle-duration-changed [:property-change-listener lifecycle/change-listener] | ||
| :delay [:setter lifecycle/scalar :coerce coerce/duration :default 0] | ||
| :on-delay-changed [:property-change-listener lifecycle/change-listener] | ||
| :on-finished [:setter lifecycle/event-handler :coerce coerce/event-handler :default nil] |
There was a problem hiding this comment.
:default nil is default behavior, no need to specify it. What's more problematic, is that presence of :default will make it go through coerce function, and nil is not a valid input to coerce/event-handler. Removing default will assign nil to finished event handler, which is what we want.
| :auto-reverse [:setter lifecycle/scalar :default false] | ||
| :on-auto-reverse-changed [:property-change-listener lifecycle/change-listener] | ||
| :on-current-time-changed [:property-change-listener lifecycle/change-listener] | ||
| :cycle-count [:setter lifecycle/scalar :coerce coerce/animation :default 1.0] |
There was a problem hiding this comment.
cycle count is int, I know it's coerced to int, but I think default should also be int to make clear.
There was a problem hiding this comment.
Yes, in this case the javadoc for Animation was wrong.
| :coerce #(if (string? %) | ||
| % | ||
| (coerce/duration %))] | ||
| :on-status-changed [:property-change-listener lifecycle/change-listener] |
There was a problem hiding this comment.
:on-status-changed will receive enum like Animation$Status/STOPPED. I'm not sure whether I want to receive it or keyword :stopped... From one hand, it's nice to have same values for :status and :on-status-changed, like we have :text and :on-text-changed for text field. On the other hand, cljfx does no preprocessing for events, and I find it valuable to have KeyEvents themselves in event handlers, because they have useful information. Although, enum is no better than keyword, and there is no additional information in this case.
What do you think?
There was a problem hiding this comment.
I think a preprocessed event could be added to another :fx/* key other than :fx/event, but this seems consistent with the rest of cljfx, if ugly.
| :duration [:setter lifecycle/scalar :coerce coerce/duration :default 400] | ||
| :from-value [:setter lifecycle/scalar :coerce double :default ##NaN] | ||
| :to-value [:setter lifecycle/scalar :coerce double :default ##NaN] | ||
| :by-value [:setter lifecycle/scalar :coerce double]))) |
There was a problem hiding this comment.
:by-value should have :default 0
| :shape [:setter lifecycle/dynamic] | ||
| :duration [:setter lifecycle/scalar :coerce coerce/duration :default 400] | ||
| :from-value [:setter lifecycle/scalar :coerce coerce/color :default nil] | ||
| :to-value [:setter lifecycle/scalar :coerce coerce/color :default nil]))) |
There was a problem hiding this comment.
should remove :defaults: nil is not a valid input for coerce/color
| (= :indefinite x) Animation/INDEFINITE | ||
| :else (int x))) | ||
|
|
||
| (defn interpolator [x] |
There was a problem hiding this comment.
should be moved to cljfx.fx.transition
| (defn animation [x] | ||
| (cond | ||
| (= :indefinite x) Animation/INDEFINITE | ||
| :else (int x))) |
There was a problem hiding this comment.
should be moved to cljfx.fx.animation
| :linear Interpolator/LINEAR | ||
| (fail Interpolator x)))) | ||
|
|
||
| (defn path-transition-orientation [x] |
There was a problem hiding this comment.
should be moved to cljfx.fx.path-transition
There was a problem hiding this comment.
Replaced with coerce/enum.
| {:fx/type view | ||
| :state state})))) | ||
|
|
||
| (fx/mount-renderer *state renderer) |
There was a problem hiding this comment.
I noticed you don't use state at all, in that case there is no need to introduce state/renderer in example, just use fx/create-component:
(fx/on-fx-thread
(fx/create-component
{:fx/type :stage
:showing true
:always-on-top true
:width 600
:height 500
:scene {:fx/type :scene
:root {:fx/type on-grid
:rows 5
:columns 2
:children (mapv
(fn [[header transition]]
(with-header
header
{:fx/type start-transition-on
:transition transition
; run each animation on a small rectangle
:desc {:fx/type :rectangle
:width 20
:height 50}}))
indefinite-animations)}}}))There was a problem hiding this comment.
Moved the state stuff to a comment; I think it's always nice to be able to use state to play with the examples.
| :playing (.play ^TranslateTransition %1) | ||
| :stopped (.stop ^TranslateTransition %1))) | ||
| lifecycle/scalar | ||
| :default :stopped]))) |
There was a problem hiding this comment.
:state is covered by :status, there is no need for such prop, right?
|
I've addressed all the changes, I'll let you resolve the change requests. Thanks for your time! |
|
Thanks! |
2 participants
Based on the feedback of #30, this PR adds support for JavaFX transitions.
The example demonstrates all transitions using
:indefinitecycles. I haven't tried transitions that are more responsive, and we should probably try them before merging this.Example gif