update guidlines #15686
update guidlines #15686
Conversation
flexsurfer
requested review from
briansztamfater,
ilmotta,
Parveshdhull,
OmarBasem and
ulisesmac
Jenkins BuildsClick to see older builds (8)
|
doc/new-guidelines.md
Outdated
| ;; bad | ||
| ;; good | ||
| (defn circle | ||
| [] | ||
| (let [opacity (reanimated/use-shared-value 1)] | ||
| [reanimated/view {:style (reanimated/apply-animations-to-style | ||
| {:opacity opacity} | ||
| style/circle-container)}])) | ||
|
|
||
| ;; good | ||
| ;; bad |
There was a problem hiding this comment.
I personally don't agree with this. Like that we would be mixing styles with view. Animation styles are still styles. There are animation styles that have 5+ style props, I don't think we should be having these in the view file.
There was a problem hiding this comment.
but its not like static styles right? its not about that you open styles ns and change style, its about defining which styles will be animated (dynamic) if you don't like map in your view you always can move it in a separate function with no problem, if you have 5+ style props it will look like (style/circle-container something-prop1 something-prop2 something-prop3 something-prop4 something-prop5 ) so it doesn't look good anyway, and better to have somethingl like
(let [opacity (reanimated/use-shared-value 1)
ra-styles {:opacity opacity}]
[ra/view (ra/apply-styles style/static ra-styles)])
There was a problem hiding this comment.
At first I was on @flexsurfer's camp, then a comment from @briansztamfater convinced me to move to @OmarBasem camp, and I was cool there ;) I think styles in general don't look good anyway, but I tend to agree with Omar a little bit more.
There was a problem hiding this comment.
could you pls share comment from @briansztamfater
There was a problem hiding this comment.
I think styles in general don't look good anyway
could you elaborate?
There was a problem hiding this comment.
i feel bad merging without @OmarBasem and @briansztamfater approves :(
There was a problem hiding this comment.
@flexsurfer sorry I somehow skipped this PR, will check it now :)
There was a problem hiding this comment.
Yeah, so we discussed this some time ago, then the discussion leaded to update the guideline as it currently is. The link to the last time we defined this is #15489 (comment)
In general terms, I agree with @OmarBasem and @ilmotta. One of the problems of not declaring animated styles in the styles file, is that it would make our components to have lot of boilerplate code for merging styles with animated styles, when we can abstract the responsibility to the style definition in the styles file and make our components look cleaner.
Also we could end up maintaining a same style for the same view on two different files, making it a bit more difficult to maintain and understand.
Regarding the naming, in the past I proposed renaming apply-animations-to-style to merge-style, the reason behind this is here #15275 (comment). But I assume that is another topic
In conclusion I don't agree with this guideline change but I can adapt, the important thing IMO is to have a guideline to follow and make it immutable at least for a long time :P
There was a problem hiding this comment.
Also, regarding his example
but its not like static styles right? its not about that you open styles ns and change style, its about defining which styles will be animated (dynamic) if you don't like map in your view you always can move it in a separate function with no problem, if you have 5+ style props it will look like
(style/circle-container something-prop1 something-prop2 something-prop3 something-prop4 something-prop5 )so it doesn't look good anyway, and better to have somethingl like(let [opacity (reanimated/use-shared-value 1) ra-styles {:opacity opacity}] [ra/view (ra/apply-styles style/static ra-styles)])
I feel that as more complex or big the animated style is, the more boilerplate the component will have just to set a style. And if we have many reanimated views in the same component, it might be worse.
(let [opacity (reanimated/use-shared-value 1)
width (reanimated/use-shared-value 100)
ra-styles {:opacity opacity
:width width
:height width}]
[ra/view {:style (ra/apply-styles style/static ra-styles)}])vs
(let [opacity (reanimated/use-shared-value 1)
width (reanimated/use-shared-value 100)]
[ra/view {:style (style/square opacity width)}])So now let's add a new view to the component that moves horizontally
(let [opacity (reanimated/use-shared-value 1)
width (reanimated/use-shared-value 100)
translate-x (reanimated/use-shared-value 0)
ra-styles-square {:opacity opacity
:width width
:height width}
ra-styles-moving {:transform [{:translateX translate-x}]
:opacity opacity}]
[rn/view
[ra/view {:style (ra/apply-styles style/static-square ra-styles-square)}]
[ra/view {:style (ra/apply-styles style/static-moving ra-styles-moving)}]])vs
(let [opacity (reanimated/use-shared-value 1)
width (reanimated/use-shared-value 100)
translate-x (reanimated/use-shared-value 0)]
[rn/view
[ra/view {:style (style/square opacity width)}]
[ra/view {:style (style/moving opacity translate-x)}]])It just feels more natural and understandable with both animated and static styles in the styles file, also a simpler guideline to follow for contributors.
There was a problem hiding this comment.
I don't like the idea of having animated styles in the style namespace, but it's true that doing this makes the view code more clean and easier to read, so I vote for having them in the style ns.
doc/new-guidelines.md
Outdated
|
|
||
| ```clojure | ||
| ;; ok | ||
| [rn/view {:flex 1 :padding-top 5}] |
There was a problem hiding this comment.
Please, add this style map inside a :style key in a map:
{:style {:flex 1 :padding-top 5}}
just to make sure it's clear that we always want that style key.
doc/new-guidelines.md
Outdated
| (defn functional-comp [atom] | ||
| [rn/text atom]) | ||
|
|
||
|
|
flexsurfer
force-pushed
the
feature/update_guidlined
branch
from
b7438ff to
edf850a
Compare
| [rn/view {:style {:flex 1 :padding-top 5}}] | ||
| ``` | ||
|
|
||
| ### Don't define properties in styles ns |
flexsurfer
force-pushed
the
feature/update_guidlined
branch
from
edf850a to
ec432c6
Compare
fixes #15552
changed ### Always apply animated styles in the style file to Neverbecause there is no real reason to mix animated styles and hide animated function inside styles namespace , better to just simplify reanimated function