New file tree by philipp-spiess · Pull Request #46602 · sourcegraph/sourcegraph-public-snapshot

philipp-spiess · 2023-01-17T17:49:40Z

Part of #12916

Todo

Add a proper feature flag
Fix the weird jumping when big section are focused
Propose a fix for the scrolling upstream
Wait for upstream to fix maybe? ➡️ We can also use the monkeypatch (I won't enable the feature flag for others until this is fixed!) for now.

Todo for later

➡️ moved to #12916

Test plan

Tested locally
You can see an example here: https://sourcegraph.slack.com/archives/C04931KQVRC/p1673961506582769
Happy to update the video if you want it but I'd like for this to get rolled into main so we can test it properly on S2

sg-e2e-regression-test-bob · 2023-01-17T17:55:57Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (+0.08 kb)	0.16% (+22.73 kb) 🔺	0.20% (+22.65 kb) 🔺	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 90c163b and 83270f4 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

felixfbecker · 2023-01-17T18:14:15Z

This is awesome! Could we introduce a Tree component to the Wildcard library that wraps the underlying open source library so that it can be easily reused in other places (like symbols sidebar, reference panel)?

philipp-spiess · 2023-01-17T18:53:31Z

@felixfbecker

Could we introduce a Tree component to the Wildcard library that wraps the underlying open source library so that it can be easily reused in other places (like symbols sidebar, reference panel)?

I think this would be AWESOME and we should do that but I’m a bit hesitant to do this just yet because I don't fully understand the different needs of these views yet.

What about we add this as a one-off for now, iron out the edge cases, maybe start thinking about what symbols and the references panel would need, and then onboard it to Wildcard?

…t Taras stood by and did not hold me back so he's also to blame

felixfbecker · 2023-01-18T17:42:25Z

+// on the folder name as well and thus the jumping is unexpected. To fix this,
+// we have to patch `.focus()` and handle the case ourselves.
+//
+// TODO: This should be fixed upstream in react-accessible-treeview.


I'd suggest making a PR there, maybe it'll get merged quickly. Monkey-patching HTMLElement.prototype.focus() seems really hacky

@felixfbecker Yeah I can look into it. We could also vendor the library, it's just two files. But I'll try to come up with a reasonable API (Maybe providing a custom focus helper? Maybe just a callback + a way to set the preventScroll option?)

Maybe I’m overthinking it, I'll just post the same fix that I did, since this is also a weird experience in the examples of this library if you start to constrain the size:

Screen.Recording.2023-01-18.at.19.11.58.mov

either upstream fix or vendoring sounds both reasonable to me :)

dgreene1/react-accessible-treeview#81

felixfbecker · 2023-01-18T17:46:45Z

@felixfbecker

Could we introduce a Tree component to the Wildcard library that wraps the underlying open source library so that it can be easily reused in other places (like symbols sidebar, reference panel)?

I think this would be AWESOME and we should do that but I’m a bit hesitant to do this just yet because I don't fully understand the different needs of these views yet.

What about we add this as a one-off for now, iron out the edge cases, maybe start thinking about what symbols and the references panel would need, and then onboard it to Wildcard?

If you can extend it to the additional use cases as a direct follow-up that makes sense. I think the explicit goal here should be to find something that is generically reusable for other areas, so you're right that we don't really know until we use it in at least two places, which only means we should do that rather sooner than later.

I could also see an argument for already reusing it in this PR for the symbols sidebar to figure out if we can make it work like that (because maybe the library is actually not well generalizable, who knows). But that's up to you, certainly good to ship improvements faster too.

philipp-spiess · 2023-01-18T18:00:00Z

I could also see an argument for already reusing it in this PR for the symbols sidebar to figure out if we can make it work like that (because maybe the library is actually not well generalizable, who knows). But that's up to you, certainly good to ship improvements faster too.

Yeah that's what I want to figure out. I had to build a bit of book-keeping to make it work well for the files use case, and I think that's fine to be kept file-tree-specific. Maybe what's reusable is the the general layout and UI (so everything beside the icons and the text that is rendered)

mrnugget

Only looked at *.go and left some comments that should be addressed, after that, as long as you tested this, I'm good withit

eseliger · 2023-01-19T13:36:37Z

+// on the folder name as well and thus the jumping is unexpected. To fix this,
+// we have to patch `.focus()` and handle the case ourselves.
+//
+// TODO: This should be fixed upstream in react-accessible-treeview.


either upstream fix or vendoring sounds both reasonable to me :)

eseliger · 2023-01-19T13:42:47Z

This looks great btw :)

sourcegraph-bot · 2023-01-23T13:43:36Z

Codenotify: Notifying subscribers in OWNERS files for diff a528304...90c163b.

Notify	File(s)
@sourcegraph/code-exploration-devs	client/wildcard/src/components/Tree/Tree.module.scss client/wildcard/src/components/Tree/Tree.story.module.scss client/wildcard/src/components/Tree/Tree.story.tsx client/wildcard/src/components/Tree/Tree.tsx client/wildcard/src/components/Tree/index.ts client/wildcard/src/components/Tree/react-accessible-treeview/LICENSE client/wildcard/src/components/Tree/react-accessible-treeview/README.md client/wildcard/src/components/Tree/react-accessible-treeview/index.tsx client/wildcard/src/components/Tree/react-accessible-treeview/utils.ts client/wildcard/src/components/index.ts
@vovakulikov	client/wildcard/src/components/Tree/Tree.module.scss client/wildcard/src/components/Tree/Tree.story.module.scss client/wildcard/src/components/Tree/Tree.story.tsx client/wildcard/src/components/Tree/Tree.tsx client/wildcard/src/components/Tree/index.ts client/wildcard/src/components/Tree/react-accessible-treeview/LICENSE client/wildcard/src/components/Tree/react-accessible-treeview/README.md client/wildcard/src/components/Tree/react-accessible-treeview/index.tsx client/wildcard/src/components/Tree/react-accessible-treeview/utils.ts client/wildcard/src/components/index.ts

philipp-spiess · 2023-01-23T14:24:53Z

It's unclear when the upstream fix will be merged (dgreene1/react-accessible-treeview#81) and I don't want to be blocked by it, so I went ahead and vendored the library for now to avoid the controversial hack. Let me know if you can think of any issues with this approach (as a nice upside we can also remove the PropTypes usage, yay!). Included the LICENSE file as well.

taras-yemets

Great work, Philipp!

valerybugakov

Awesome work! Excited to start using it on S2 as soon as it becomes available 🔥

valerybugakov · 2023-01-24T12:01:26Z

+    }
+
+    return (
+        <Tree<TreeNode>


Nice typed stuff

Part of #46602 This adds single-child folder expansion to the new tree view based on how VS Code does it 🎉 Since the implementation of the tree is append-only, only a bit of bookkeeping is needed to detect single-child expansion scenarios and change the tree entries accordingly. I've left various comments in the code which should explain the implementation for the curious.

philipp-spiess added 6 commits January 16, 2023 16:02

Add option to recursivly load parent folders to GraphQL query

4b8fd1a

Add first version of new file tree

dc0ca70

Allow files to be opened again

b279c05

Remove links from tab order

e6515da

Various UI tweaks

84d63b1

Various linter and type fixes

eba91b3

cla-bot Bot added the cla-signed label Jan 17, 2023

github-actions Bot added the team/code-exploration Issues owned by the Code Exploration team label Jan 17, 2023

philipp-spiess added 3 commits January 18, 2023 12:11

Fix scrolling behavior by patching DOM APIs. I'm not proud of this bu…

f15fccf

…t Taras stood by and did not hold me back so he's also to blame

Add telemetry

4565015

Add feature flag

353fd7d

philipp-spiess self-assigned this Jan 18, 2023

philipp-spiess requested review from a team January 18, 2023 15:35

philipp-spiess marked this pull request as ready for review January 18, 2023 15:37

Fix linter issue

5a45a7b

felixfbecker reviewed Jan 18, 2023

View reviewed changes

Fix typ issue

a0d55f7

mrnugget approved these changes Jan 19, 2023

View reviewed changes

Comment thread cmd/frontend/graphqlbackend/git_tree.go Outdated

Comment thread cmd/frontend/graphqlbackend/git_tree_entry.go Outdated

eseliger reviewed Jan 19, 2023

View reviewed changes

philipp-spiess mentioned this pull request Jan 19, 2023

Scroll to leaf nodes instead of the branches to avoid janky scrolling behavior dgreene1/react-accessible-treeview#81

Merged

philipp-spiess added 2 commits January 19, 2023 17:37

Adress Go feedback

8038ca7

Merge remote-tracking branch 'origin/main' into ps/new-tree-sidebar

bc774ed

philipp-spiess added 2 commits January 23, 2023 12:42

Merge remote-tracking branch 'origin/main' into ps/new-tree-sidebar

6166cd9

Graduate Tree into a Wildcard component

7bbe281

philipp-spiess added 2 commits January 23, 2023 14:48

Clean up CSS

ea23e76

Re-add removed CSS class

1350df7

philipp-spiess requested a review from taras-yemets January 23, 2023 14:05

Vendor react-accessible-treeview

af87984

Remove hack

1978fa5

philipp-spiess force-pushed the ps/new-tree-sidebar branch from 9017181 to 1978fa5 Compare January 23, 2023 14:25

Add support for submodule rendering

d9ff0ac

taras-yemets approved these changes Jan 24, 2023

View reviewed changes

valerybugakov approved these changes Jan 24, 2023

View reviewed changes

philipp-spiess added 2 commits January 24, 2023 14:24

Handle error case

51fa31a

Adress review feedback

90c163b

philipp-spiess merged commit ab00308 into main Jan 24, 2023

philipp-spiess deleted the ps/new-tree-sidebar branch January 24, 2023 14:37

This was referenced Jan 30, 2023

New file tree: Navigation tweaks #47095

Merged

Treeview: Expand single-child folders in the new tree view #47117

Merged

Conversation

philipp-spiess commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo

Todo for later

Test plan

Uh oh!

sg-e2e-regression-test-bob commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

felixfbecker commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipp-spiess commented Jan 17, 2023

Uh oh!

felixfbecker Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

philipp-spiess Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

philipp-spiess Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

eseliger Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

philipp-spiess Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

felixfbecker commented Jan 18, 2023

Uh oh!

philipp-spiess commented Jan 18, 2023

Uh oh!

mrnugget left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eseliger Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

eseliger commented Jan 19, 2023

Uh oh!

sourcegraph-bot commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipp-spiess commented Jan 23, 2023

Uh oh!

taras-yemets left a comment

Choose a reason for hiding this comment

Uh oh!

valerybugakov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valerybugakov Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

philipp-spiess commented Jan 17, 2023 •

edited

Loading

sg-e2e-regression-test-bob commented Jan 17, 2023 •

edited

Loading

felixfbecker commented Jan 17, 2023 •

edited

Loading

sourcegraph-bot commented Jan 23, 2023 •

edited

Loading