Page MenuHomePhabricator

[web] Add PossiblyEmptyNavStateInfoBar to handle collapse animation in the topbar when no community is selected
ClosedPublic

Authored by inka on Feb 23 2023, 12:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 4, 12:12 PM
Unknown Object (File)
Feb 23 2024, 6:58 AM
Unknown Object (File)
Feb 23 2024, 6:57 AM
Unknown Object (File)
Feb 23 2024, 5:32 AM
Unknown Object (File)
Feb 23 2024, 5:02 AM
Unknown Object (File)
Feb 23 2024, 1:46 AM
Unknown Object (File)
Feb 22 2024, 11:54 PM
Unknown Object (File)
Feb 21 2024, 12:59 PM
Subscribers

Details

Summary

Issue: https://linear.app/comm/issue/ENG-2740/add-navigation-state-info-to-the-top-bar
This component is needed as a wrapper for NavStateInfoBar, because sometimes no community is selected in the drawer. In such case, the part of the topbar that shows the navigation info is supposed to collapse. This cannot be handled
by NavStateInfoBar directly, because it calls hooks that take ThreadInfo as an argument (which in such case is undefined), and we cannot call hooks conditionally.
We also want the navigation state info to be visible during the collapse animation. This is why PossiblyEmptyNavStateInfoBar has a threadInfo in it's state. It remembers the previous threadInfo for the time of the animation. Then
it is set to null. If it was not being set to null after the animation, during the expand animation we would see for a fragment of a second the previous navigation state, that would suddenly be updated, and that looks glitchy.

There is a slight glitch sometimes when switching from the Calendar tab (with no community selected) to the Chat tab, but this is caused by ChatThreadList component being very low, and influencing the overall performance of the app.
It had been decided that it's fine. We have tasks to improve the performance of the ChatThreadList: ENG-2225 and ENG-891.

The styling will be updated in future diffs

Test Plan

Run web app, added PossiblyEmptyNavStateInfoBar component to the Topbar component, checked that is behaves as expected.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 23 2023, 1:05 AM
Harbormaster failed remote builds in B16794: Diff 22963!
inka requested review of this revision.Feb 23 2023, 1:11 AM
web/navigation-panels/nav-state-info-bar.react.js
46 ↗(On Diff #22963)

Shouldn't this state initially be set to props.threadInfoInput? Doesn't it save one extra re-render of the memo below, when the initial props.threadInfoInput is non-null?

48–56 ↗(On Diff #22963)

I'm not an expert, but two things

  • It is a good practice that the setTimeout handle should be cleared with clearTimeout in the effect cleanup function. Otherwise, you might encounter a glitch (prop=null -> setTimeout -> prop=nonNull -> setState(nonNull) -> 200ms passed -> setState(null).
  • I'm not sure if any of these examples apply here but I suspect that this effect might be unnecessary: https://beta.reactjs.org/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes - regardless of that, I consider this article worth reading anyway ;)
web/navigation-panels/nav-state-info-bar.react.js
46 ↗(On Diff #22963)

I don't think so, since the Effect hook is always called at the first render if I'm not mistaken.
But this one re-render can be avoided by both setting here sate to props.threadInfoInput and adding a condition in the effect hook, something like:

if (props.threadInfoInput && props.threadInfoInput !== threadInfo) {
      setThreadInfo(props.threadInfoInput);
} else if( !props.threadInfoInput ){
...

I guess it won't hurt.

48–56 ↗(On Diff #22963)

We could do this without using the Effect hook, but then we are not protected against the glitch you mentioned. So the effect is needed, but I didn't write the one thing it actually is needed for 🤦‍♀️
Thank you for the article, it's very insightful, I really liked it!

web/navigation-panels/nav-state-info-bar.react.js
46 ↗(On Diff #22963)

Actually the same applies to when !!threadInfoInput === false so it has to be

if(props.threadInfoInput !== threadInfo) {
  if ( props.threadInfoInput ) {
      setThreadInfo(props.threadInfoInput);
  } else if ( !props.threadInfoInput ){
    ...
  }
}
web/navigation-panels/nav-state-info-bar.react.js
46 ↗(On Diff #22963)

This 'if' definitely won't hurt.

the Effect hook is always called at the first render if I'm not mistaken.

It is called indeed, but the re-render occurs only if state changes as a consequence of executing the Effect.

Current scenario:

  1. initialProps = nonNull
  2. initialState = null
  3. component is initially rendered
  4. effect is Called: setState(null -> nonNull) - state changed
  5. change of state triggers re-render

My suggestion:

  1. initialProps = nonNull
  2. initialState = initialProps = nonNull
  3. component is initially rendered
  4. effect is Called: setState(nonNull -> nonNull) - setState called, but state itself did not change
  5. no need to re-render

And adding that if is a good idea!

Found an article describing what I mean: https://pgarciacamou.medium.com/react-doesnt-always-trigger-a-re-render-on-setstate-4644212560a

Also created a JSFiddle that proves my point: https://jsfiddle.net/xLzrv9td/

48–56 ↗(On Diff #22963)

We could do this without using the Effect hook, but then we are not protected against the glitch you mentioned.

Yes, you're right. Clearing timeout would be very tricky in this case. So let's just add clearTimeout and we're done.

web/navigation-panels/nav-state-info-bar.react.js
46 ↗(On Diff #22963)

Ahh, right. Thank you for all of that

bartek added inline comments.
web/navigation-panels/nav-state-info-bar.react.js
58 ↗(On Diff #23316)

Wow, at first I was confused with conditionally returning a cleanup function, but looks like this is legit.

This revision is now accepted and ready to land.Mar 1 2023, 2:15 AM