feat(TreeView): enabled opt-in animations#11842
Conversation
|
Preview: https://patternfly-react-pr-11842.surge.sh A11y report: https://patternfly-react-pr-11842-a11y.surge.sh |
thatblindgeye
force-pushed
the
iss11841_treeviewAnims
branch
from
f0fe4da to
0427a24
Compare
There was a problem hiding this comment.
Code looks good - I see animations on all three component types. Do we have a way to disable Accordion and Nav animations if they don't want them? I don't see hasAnimation on those. Not blocking, but I could see someone coming in and bugging folks about it.
So far the React opt-ins have been more to avoid a breaking change for consumers, with the plan to remove those props and make animations the default in the next breaking change. There is a "killswitch" class from core (can't remember the name - "pf-m-no-motion" or something I want to say?) that would effectively disable animations. |
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
I don't have an issue with this implementation.
I don't know about the future proposed change to when children of a TreeView item are rendered... I might be worried about performance of TreeViews at a larger scale if all children are always present on the page rather than rendered dynamically. On the backend, it's completely reasonable to wait to retrieve children of collapsed TreeViews items until they are expanded for performance reasons. Imagine a Tree view with thousands of leaf nodes, for example. Tree views are a way to only load nodes when they are needed.
srambach
left a comment
srambach
left a comment
There was a problem hiding this comment.
From a CSS perspective, looks good. Tree view animation looks decent even at 20x cpu loading.
Accordion and nav look good too.
thatblindgeye
force-pushed
the
iss11841_treeviewAnims
branch
from
0427a24 to
6aabf1e
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
5 participants
What: Closes #11841
Accordion from patternfly/patternfly#7539 and Nav from patternfly/patternfly#7542 should also be set without any further React work, but can use confirmation on this
Additional issues: