Conversation
joschect
requested review from
dzearing,
ecraig12345,
kenotron,
kkjeer and
oengusmacinog-zz
|
I made similar updates to |
|
add test |
|
@msft-github-bot auto merge after Jon writes a test. |
| animation-duration: 0.167s; | ||
| animation-fill-mode: both; | ||
| animation-name: keyframes from{opacity:0;}to{opacity:1;}; | ||
| animation-timing-function: cubic-bezier(.1,.25,.75,.9); |
There was a problem hiding this comment.
Why this animation is removed? Is it not breaking the animation?
There was a problem hiding this comment.
I believe that this is actually fixing a bug, a currently rendered panel should not have an animation on it. The code reads like the animation should only be added when it's animated in or out. The test as it's written just creates a panel that's already open, which to me means that it shouldn't animate . We can add some extra stuff to check for that. Animations definitely do work though.
There was a problem hiding this comment.
Very interesting that a change of the isOpen from being a state property to a class private property would change a screenshot in that manner, even if the explanation you gave makes perfect sense. Anyways I am glad that everything still works as expected.
| animation-duration: 0.167s; | ||
| animation-fill-mode: both; | ||
| animation-name: keyframes from{opacity:0;}to{opacity:1;}; | ||
| animation-timing-function: cubic-bezier(.1,.25,.75,.9); |
There was a problem hiding this comment.
Very interesting that a change of the isOpen from being a state property to a class private property would change a screenshot in that manner, even if the explanation you gave makes perfect sense. Anyways I am glad that everything still works as expected.
|
@dzearing I have added the test |
|
Component perf results:
|
|
@kkjeer I think that this is probably ready to go in! |
|
🎉 Handy links: |
6 participants
Pull request checklist
$ npm run changeDescription of changes
Currently the panel will call on dismiss twice when it is closed. I believe that this fix is one of the only ways to get it working without breaking existing functionality. As a possible other fix we could change the comment so that it explicitly states that ondismiss will get called twice. This code change doesn't quite seem right but I can't think of any other way of fixing this bug. Suggestions are welcome.
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow