Stats: Update panel animations #94756
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~41 bytes added 📈 [gzipped]) Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
matticbot
added
the
[Status] Needs Review
| <FeedbackPanel isOpen={ isFloatingPanelOpen } clickHandler={ handleButtonClick } /> | ||
| <FeedbackPanel | ||
| isOpen={ isFloatingPanelOpen } | ||
| animationClassName={ animationName } |
There was a problem hiding this comment.
animationClassName is an implementation detail we probably want to hide from the caller, also it would make it harder to reuse the FeedbackPanel with all the bells and whistles.
There was a problem hiding this comment.
This is similar to my comment below about the animation and the visibility not being 1-to-1. I see what you mean but I'm not sure the best approach here. If we want to manage the animation internally, I think we'd need to tweak it to include a callback so that the caller knows when the animation has completed.
There was a problem hiding this comment.
I guess it's a solvable problem with my suggestion above. Yeah, there are events we could potentially listen to so that we know precisely, but I'm not sure whether it's worth it.
There was a problem hiding this comment.
animationClassName is an implementation detail we probably want to hide from the caller
I agree with @kangzj. That may be too coupling.
| function dismissFloatingPanel() { | ||
| const delay = isFloatingPanelOpen ? FEEDBACK_PANEL_ANIMATION_DELAY_EXIT : 0; | ||
| setAnimationName( FEEDBACK_PANEL_ANIMATION_NAME_EXIT ); | ||
| return new Promise( ( resolve ) => { | ||
| setTimeout( () => { | ||
| setIsFloatingPanelOpen( false ); | ||
| resolve( '' ); | ||
| }, delay ); | ||
| } ); | ||
| } |
There was a problem hiding this comment.
The code here is very confusing. None of the function called here is aync but it is wrapped with Promise. That said, it is not guaranteed that the the panel is closed on resolve.
There was a problem hiding this comment.
This function was put together following the example from MDN on using promises. The goal was to have the dismiss.then() pattern (like the fetch API) so it was clear that follow up actions happened once the panel was closed.
My understanding is we do not need an async label here as we aren't using the async/await paradigm. Unless I'm misunderstanding things.
That said, it is not guaranteed that the the panel is closed on resolve.
How do you mean? I think it is guaranteed to be closed once we call setIsFloatingPanelOpen( false ).
There was a problem hiding this comment.
JavaScript is a single threaded language, and it's not possible to create async actions ourselves, which means the function is not provided by browser or system, we are not able to create real async functions, because they resolve instantly.
In the code above, what's actually working is just the timeout, and that is all. It doesn't wait for the panel to close, because the operation here is just not async (I mean it is async but not really the case for setIsFloatingPanelOpen tho). Even if it is, we should either use await or chain it with .then to be able to carry on the actions after promise resolve.
With the following code, it could achieve the same result:
useEffect( () => {
// if ( ! isPending && ! isError && shouldShowFeedbackPanel ) {
setTimeout( () => {
setIsFloatingPanelOpen( true );
}, FEEDBACK_PANEL_PRESENTATION_DELAY );
// }
}, [ isPending, isError, shouldShowFeedbackPanel ] );
function dismissFloatingPanel() {
const delay = isFloatingPanelOpen ? FEEDBACK_PANEL_ANIMATION_DELAY_EXIT : 0;
setAnimationName( FEEDBACK_PANEL_ANIMATION_NAME_EXIT );
return setTimeout( () => {
setIsFloatingPanelOpen( false );
}, delay );
}
const handleButtonClick = ( action: string ) => {
switch ( action ) {
case ACTION_SEND_FEEDBACK:
dismissFloatingPanel();
setTimeout( () => setIsOpen( true ), FEEDBACK_PANEL_ANIMATION_DELAY_EXIT );
break;
case ACTION_DISMISS_FLOATING_PANEL:
dismissFloatingPanel();
updateFeedbackPanelHibernationDelay();
trackStatsAnalyticsEvent( `stats_feedback_${ ACTION_DISMISS_FLOATING_PANEL }` );
break;
case ACTION_LEAVE_REVIEW:
dismissFloatingPanel();
window.open( FEEDBACK_LEAVE_REVIEW_URL );
break;
// Ignore other cases.
}
};There was a problem hiding this comment.
I found this article quite helpful: https://www.freecodecamp.org/news/javascript-asynchronous-operations-in-the-browser/
There was a problem hiding this comment.
JavaScript is a single threaded language, and it's not possible to create async actions ourselves, which means the function is not provided by browser or system, we are not able to create real async functions, because they resolve instantly.
This doesn't quite fit with my reading of the docs. My understanding is that Promises are the recommended way to wrap callback-based APIs like setTimeout() so they can be used in chaining. The MDN docs show two examples of this here, one which is specifically showing it as a wrapper for setTimeout().
In terms of single threaded-ness, I believe I follow your point. I understand we aren't getting async execution here. Instead we are scheduling a callback with the event loop. The goal with wrapping the callback in a Promise here is to allow for use of the chaining syntax, which keeps the animation timing central to the single dismissFloatingPanel() function while allowing us run code after the animation.
The code you shared would accomplish this as well but I believe it to be more complicated. For example:
case ACTION_SEND_FEEDBACK:
dismissFloatingPanel();
const delay = isFloatingPanelOpen ? FEEDBACK_PANEL_ANIMATION_DELAY_EXIT : 0;
setTimeout( () => setIsOpen( true ), delay );
break;
Vs:
case ACTION_SEND_FEEDBACK:
dismissFloatingPanel().then( () => {
setIsOpen( true );
} );
break;
Note that we'd need the same timing-related code when handling the "leave review" action as well.
There was a problem hiding this comment.
In the code above, what's actually working is just the timeout, and that is all.
I understand the point you are making here. The timeout is what makes this work. I just want to be clear that this is the recommended approach for wrapping callback-based APIs per the MDN docs. If we don't want to use this approach, I'm fine with that. I just want to make sure I'm not misunderstanding something important here.
It doesn't wait for the panel to close, because the operation here is just not async (I mean it is async but not really the case for setIsFloatingPanelOpen tho). Even if it is, we should either use await or chain it with .then to be able to carry on the actions after promise resolve.
I think the important notes here are:
- If you capture and log the promise, it starts as pending, so the proper order of operations is observed.
- The call to
setIsFloatingPanelOpen()executes after the specified delay, meaning the animation should have the necessary time to complete. - We are making use of the
.then()syntax at the call site to handle opening a new tab/window or present the feedback modal.
Also, just to be clear, I'm not trying to be difficult here. I'm happy to do it either way. I just want to make sure I'm understanding any nuances that I might be missing so I properly grok things going forward.
There was a problem hiding this comment.
My understanding is that Promises are the recommended way to wrap callback-based APIs like setTimeout() so they can be used in chaining.
Yes you are right. I think I got what you were trying to achieve now. I still don't quite like what's happening in here tho. If we are using the timeouts, then use timeouts, it's an estimation of how long the animation would take and then take next actions. The promise only saves setting up another timeout, but made the code harder to parse. Also setIsFloatingPanelOpen and setIsOpen could be in the same timer really.
case ACTION_SEND_FEEDBACK:
dismissFloatingPanel();
const delay = isFloatingPanelOpen ? FEEDBACK_PANEL_ANIMATION_DELAY_EXIT : 0;
setTimeout( () => { setIsOpen( true ); setIsFloatingPanelOpen( false ); }, delay );
break;My suggestion would be that, we either do it precisely - listen on animation start / end events or we do it simply - relying on timers, and getting rid of the promises.
Sorry to be a pain here. The reason I'm having long discussions is that you are passionate about code quality and so am I, and this is a great opportunity to have a very specific and in-depth discussion. Plus the PR doesn't seem to be urgent.
@dognose24 I wonder what you think?
There was a problem hiding this comment.
Wrapping the setTimeout within Promise would be a viable way to delay the callback execution, which may benefit chained synchronous functions more. Let's make the code as readable as possible unless it is necessary to become complex.
As the function dismissFloatingPanel is shared to achieve the delay, is passing a callback enough?
function dismissFloatingPanel( callback ) {
const delay = isFloatingPanelOpen ? FEEDBACK_PANEL_ANIMATION_DELAY_EXIT : 0;
setTimeout( () => {
setIsFloatingPanelOpen( false );
callback();
}, delay );
}
There was a problem hiding this comment.
Note that we'd need the same timing-related code when handling the "leave review" action as well.
I am not sure about this. It looks a little unintuitive to wait for the animation to open a new tab. 🤔
There was a problem hiding this comment.
As the function dismissFloatingPanel is shared to achieve the delay, is passing a callback enough?
As Kevin mentioned, "this is the recommended approach for wrapping callback-based APIs per the MDN docs" I think 🤔
| const translate = useTranslate(); | ||
| const [ animationClassName, setAnimationClassName ] = useState( | ||
| FEEDBACK_PANEL_ANIMATION_NAME_ENTRY | ||
| ); | ||
|
|
There was a problem hiding this comment.
It feels like we could achieve the goal of the PR by adding the following lines here:
useEffect( () => {
if ( isOpen ) {
setAnimationClassName( FEEDBACK_PANEL_ANIMATION_NAME_ENTRY );
} else {
setAnimationClassName( FEEDBACK_PANEL_ANIMATION_NAME_EXIT );
}
}, [ isOpen ] );There was a problem hiding this comment.
I don't believe this would work because it's the panel visibility and the animation are not 1-to-1 state. If the isOpen value is false, nothing is drawn. Because the animation requires time to run, it needs to be a two-step process.
- Trigger the animation via the classname.
- Close the panel after the animation has run.
There aren't any hooks that would alert us when the animation has completed which is why the code uses a timeout with a small delay.
There was a problem hiding this comment.
I think I got what you are talking about, so I put together some quick code to show how it could be done within the floating panel itself.
https://github.com/Automattic/wp-calypso/compare/try/panel-animation?expand=1
There was a problem hiding this comment.
I agree with @kangzj here, as the animation would be better controlled in the operating component itself. Leaving the animation class toggling to the parent might be coupled. Ideally, the parent should only control if the element is open.
We can also use isFloatingPanelOpen to determine the mounting of the FeedbackPanel
{ isFloatingPanelOpen && <FeedbackPanel clickHandler={ handleButtonClick } /> }
and utilize the React lifecycle componentDidMount/componentWillUnmount to approach the animation.
| @@ -161,6 +157,7 @@ function FeedbackCard( { clickHandler }: FeedbackPropsInternal ) { | |||
| function StatsFeedbackController( { siteId }: FeedbackProps ) { | |||
| const [ isOpen, setIsOpen ] = useState( false ); | |||
There was a problem hiding this comment.
No related to the PR, but I guess isOpen could use a better name since there are two open statuses for the child components.
There was a problem hiding this comment.
Agreed. This was named before the floating panel was merged so there was only a single view to track at that time. It definitely makes sense to update the naming now.
| function dismissFloatingPanel() { | ||
| const delay = isFloatingPanelOpen ? FEEDBACK_PANEL_ANIMATION_DELAY_EXIT : 0; | ||
| setAnimationName( FEEDBACK_PANEL_ANIMATION_NAME_EXIT ); | ||
| return new Promise( ( resolve ) => { | ||
| setTimeout( () => { | ||
| setIsFloatingPanelOpen( false ); | ||
| resolve( '' ); | ||
| }, delay ); | ||
| } ); | ||
| } |
There was a problem hiding this comment.
Wrapping the setTimeout within Promise would be a viable way to delay the callback execution, which may benefit chained synchronous functions more. Let's make the code as readable as possible unless it is necessary to become complex.
As the function dismissFloatingPanel is shared to achieve the delay, is passing a callback enough?
function dismissFloatingPanel( callback ) {
const delay = isFloatingPanelOpen ? FEEDBACK_PANEL_ANIMATION_DELAY_EXIT : 0;
setTimeout( () => {
setIsFloatingPanelOpen( false );
callback();
}, delay );
}
| const translate = useTranslate(); | ||
| const [ animationClassName, setAnimationClassName ] = useState( | ||
| FEEDBACK_PANEL_ANIMATION_NAME_ENTRY | ||
| ); | ||
|
|
There was a problem hiding this comment.
I agree with @kangzj here, as the animation would be better controlled in the operating component itself. Leaving the animation class toggling to the parent might be coupled. Ideally, the parent should only control if the element is open.
We can also use isFloatingPanelOpen to determine the mounting of the FeedbackPanel
{ isFloatingPanelOpen && <FeedbackPanel clickHandler={ handleButtonClick } /> }
and utilize the React lifecycle componentDidMount/componentWillUnmount to approach the animation.
| function dismissFloatingPanel() { | ||
| const delay = isFloatingPanelOpen ? FEEDBACK_PANEL_ANIMATION_DELAY_EXIT : 0; | ||
| setAnimationName( FEEDBACK_PANEL_ANIMATION_NAME_EXIT ); | ||
| return new Promise( ( resolve ) => { | ||
| setTimeout( () => { | ||
| setIsFloatingPanelOpen( false ); | ||
| resolve( '' ); | ||
| }, delay ); | ||
| } ); | ||
| } |
There was a problem hiding this comment.
Note that we'd need the same timing-related code when handling the "leave review" action as well.
I am not sure about this. It looks a little unintuitive to wait for the animation to open a new tab. 🤔
Related to https://github.com/Automattic/red-team/issues/173
Proposed Changes
Controller manages animation via classnames. In this way the panel is always animated smoothly offscreen regardless of which button/action is triggered. Previously only the "x" and Dismiss actions would trigger an animation.
Why are these changes being made?
Keeps panel behaviour consistent.
Testing Instructions
Note: If you check out
d18c92a8locally there's an extra button to show/hide the panel for easier testing without submitting or throttling.Pre-merge Checklist