-
Notifications
You must be signed in to change notification settings - Fork 985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#18461] Update docs about Reanimated apply-animations-to-style #18462
Conversation
[reanimated/view {:style (reanimated/apply-animations-to-style | ||
{:opacity opacity} | ||
style/circle-container)}])) | ||
[reanimated/view {:style [{:opacity opacity} | ||
style/circle-container]}])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guideline is about having animated styles in a styles ns, just used the vector syntax
doc/new-guidelines.md
Outdated
@@ -230,6 +229,49 @@ Properties must be set on view level | |||
[reanimated/view {:style (style/circle-container opacity)}])) | |||
``` | |||
|
|||
### `apply-animations-to-style` vs `[]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new guideline 👁️
@ilmotta @briansztamfater @smohamedjavid @J-Son89 I'd appreciate your feedback a lot in order to make this guideline clear and understandable :) |
Jenkins Builds
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally forgot about suggesting changes in the guidelines in that PR introducing the vector syntax, so thank you for coming back to this.
doc/new-guidelines.md
Outdated
@@ -230,6 +229,49 @@ Properties must be set on view level | |||
[reanimated/view {:style (style/circle-container opacity)}])) | |||
``` | |||
|
|||
### `apply-animations-to-style` vs `[]` | |||
|
|||
`apply-animations-to-style` is a function wrapping `reanimated/use-animated-style` to make it work in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ulisesmac, the guidelines have grown and the more we increase it, the less devs will read it over and over. I have seen this problem dozens of times already, people missing what's already in the guidelines.
So in that spirit, I'm thinking if we can make this more concise and to the point.
Here's just one suggestion. I removed a little bit of code from the example and changed the text to start with an actionable verb prefer.
### Pass a vector of styles to Reanimated view
Prefer to pass a vector of styles to `react-native.reanimated/view` `:style`
prop instead of using `apply-animations-to-style` directly. For more details, check out Reanimated docs about [inline
styles](https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/your-first-animation/#defining-a-shared-value)
and [useAnimatedStyle](https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/animating-styles-and-props/#animating-styles)
for more information.
(defn f-view []
(let [scroll-x (reanimated/use-shared-value 0)
opacity (reanimated/interpolate scroll-x [0 45 50] [1 1 0])]
[reanimated/view
;; bad
{:style (reanimated/apply-animations-to-style
{:opacity opacity
:transform [{:translate-x scroll-x}]}
{:flex-direction :row})}
;; good
{:style [{:opacity opacity
:transform [{:translate-x scroll-x}]}
{:flex-direction :row}]}
;; other valid and good variants
{:style [{:opacity opacity}
{:transform [{:translate-x scroll-x}]}
{:flex-direction :row}]}
{:style {:opacity opacity
:transform [{:translate-x scroll-x}]
:flex-direction :row}}]))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with this. When I check the guidelines, I usually do a quick search and look for something like 'do not do this, do that instead.' Thus, the more concise the guideline, the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion @ilmotta !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this @ulisesmac! I approved this as seems good enough for me, but also agree with @ilmotta that we can improve the guideline to be more concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding it in the guidelines 🙌 . Except minor verbal changes (straight to point) suggested by @ilmotta. LGTM!
de3d091
to
8fd504f
Compare
@status-im/mobile-devs Just adding the missing guideline about vector syntax in the previous PR: #18381 👀 |
fixes #18461
Summary
Just a guidelines doc update about the vector syntax
status: ready