file tree: open relevant folders on nav #233
Conversation
Wattenberger
requested review from
mattrothenberg,
jaked and
idan
as code owners
includes: upgrade react-vtree to v3
Wattenberger
force-pushed
the
aw/file-tree
branch
from
22c9911 to
54a686c
Compare
|
Tested locally on my end and seems to be working just fine! No problems with the minimap. |
components/Sidebar.tsx
Outdated
| filteredFiles, | ||
| updatedContents, | ||
| numberOfFiles < 20, | ||
| activeFilePath, | ||
| query, | ||
| query.branchPath.join("/"), | ||
| branchName, |
There was a problem hiding this comment.
I think you need to keep the dependencies on activeFilePath and query since they're referenced in getNodeData. or maybe just drop the useMemo, it's not accomplishing anything.
There was a problem hiding this comment.
hmm, what am I missing here:
- the
query.branchPath.join("/"),dep should cover active path changes, right? - and the memo is definitely not crucial, but should help a bit with perf. mayhaps it's not worth the extra overhead!
There was a problem hiding this comment.
oh hm you are right about query.branchPath.join("/") covering activePath, I didn't think that through.
I think this might be one of those cases where the memoized thing is inexpensive so the cost of memoization is nearly the same — I think it's worth dropping these generally because it's so easy to create inadvertent update bugs by leaving off a dependency, but no strong feeling about this one in particular, I'm gappy either way.
There was a problem hiding this comment.
that's fair! I'll remove it to keep the code nice and goncise
This involves preserving the existing open state (don't want to close folders that are already open as the user navigates) and fixing some timing / dependency bugs to make sure the nav scrolls to the newly open file (eg. if clicked to in the minimap).
I also upgraded us to react-vtree to v3 for perf & a cleaner API - it's been in beta since 2020 😅 and is purportedly (and in testing) stable enough to use Lodin/react-vtree#83