-
Notifications
You must be signed in to change notification settings - Fork 107
feat(ui-tree-browser): add animation prop to TreeBrowser #2137
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
feat(ui-tree-browser): add animation prop to TreeBrowser #2137
Conversation
|
animationFillMode: 'forwards', | ||
animationTimingFunction: 'ease-out', | ||
animationDelay: '0.2s', | ||
...(state.animation !== false && { |
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.
just a nitpic: you don't have to check with !==
, since animation is a boolean
itself. It would look better to use just the value itself: state.animation && {...
Another nitpic of mine is false && {}
fill resolve to false
. Even so spreading false
into an object works for some reason (that's js for you :D ) it doesn't make much sense. You could write these expressions like:
...(state.animation !== false && { | |
...(state.animation ? {<animation code>} : {} |
This way it is clearly stated what you indend in both branches
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.
@HerrTopi thanks, I made the changes you suggested
animationDuration: '0.2s', | ||
animationFillMode: 'forwards', | ||
animationTimingFunction: 'ease-out', | ||
...(state.animation !== false && { |
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.
Same as prev comments
db13291
to
9a3e3ec
Compare
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.
looks good, just 1 small change
* @module | ||
*/ | ||
const TreeBrowserContext = createContext<TreeBrowserContextType>({ | ||
animation: false |
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 think this should be true
for that very edge case if someone uses these components outside of a TreeBrowser
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.
@matyasf thanks, I fixed that.
9a3e3ec
to
6bb7a8e
Compare
INSTUI-4683
ISSUE:
TEST PLAN: