Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Svelte: First run on file tree visual update #62585

Merged
merged 11 commits into from May 15, 2024
Merged

Conversation

vovakulikov
Copy link
Contributor

@vovakulikov vovakulikov commented May 10, 2024

Part of #62001

This PR mostly updates the visual representation of the file tree side panel. It also adds a few action buttons to the file tree header.

Expanded Collapsed
Screenshot 2024-05-13 at 20 31 35 Screenshot 2024-05-13 at 20 31 57

Loom Demo https://www.loom.com/share/0e72844210934a86868dd722e9c2b3e9

Todo

  • Implement a collapsed layout for the file tree panel
  • Add go to the root button
  • Add search file shortcut button
  • Update file tree spacing and nesting layout
  • Update file tree colors for selected states
  • Add move scope to the directory button to the file item
  • Add vertical lines for the nesting levels (Instead I adjusted the paddings for nested levels since vertical lines were too noise on some deep nested levels)

Test plan

  • Manual testing of side file tree panel

@vovakulikov vovakulikov self-assigned this May 10, 2024
@cla-bot cla-bot bot added the cla-signed label May 10, 2024
@vovakulikov vovakulikov requested a review from a team May 10, 2024 03:32
@camdencheek
Copy link
Member

@vovakulikov I see you requested review, but this is still in draft state. What kind of review are ya looking for?

@vovakulikov
Copy link
Contributor Author

@camdencheek you can ignore it for now. For some reason I thought that since this is in draft mode you won't be notified

@vovakulikov
Copy link
Contributor Author

@taiyab I fixed the file icon color for selected states, I haven't added vertical lines (I tried and it looked a bit crowded with additional elements) but I think our paddings for nesting levers were off, I think I fixed them now, (basically I did repeat nesting from JetBrains IDEs)

Screenshot 2024-05-13 at 15 20 17

@vovakulikov vovakulikov requested a review from taiyab May 13, 2024 18:30
@vovakulikov vovakulikov marked this pull request as ready for review May 13, 2024 18:34
Copy link
Contributor

@taiyab taiyab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I've added some comments on the Loom for feedback.

(The drag to collapse menu is genius).

@taiyab taiyab requested a review from a team May 13, 2024 21:21
@vovakulikov
Copy link
Contributor Author

@taiyab echo your comments here, just to keep it in one place

How come all the controls/buttons get crowded onto one line? Flexbox problem?

This actually was intentional, because on the big screens, there will be enough space for these controls, we could either

  • just have it in two rows always
  • set a bit bigger minimal sizes for the rev and search files buttons to switch to one row with sizes you're comfortable with

I'm happy to hide the "collapse" button (maybe keep the expand one), so by default, at least for now, the way to collapse the panel is to drag it down until it pops away.

I remember we had a big conversation that having non-jumping controls for show/hide actions is very vital. If @fkling is happy here I can live with not having this but I would prefer to still have it

@vovakulikov
Copy link
Contributor Author

On a bit of technical note (echo it from Slack to keep convos in one place)

Before this PR, I thought that file tree providers knew about each other as we go from folder to folder, but it seems they don't, and the only way it works smoothly at the moment is because of our cache on the rural level. This doesn't cause any big problems at the moment, but I think if we need to have more interaction about which folder we want to open and when we probably should have something like a graph rather than a plain list of data sources.

I've seen parents linking in the codebase (I think these arguments are not used and are obsolete at the moment), so @fkling probably has already thought about this, but I am curious what problem you had with this.

@taiyab
Copy link
Contributor

taiyab commented May 14, 2024

just have it in two rows always

This please.

that having non-jumping controls for show/hide actions is very vital

I was proposing this as a temporary solution, as I was intending to actually have the button to the left of the file header so it always maintained its position. However, after some experimenting, we can slightly adjust what you've done instead:

CleanShot 2024-05-14 at 05 47 51@2x

Designs: https://www.figma.com/file/EPgl0qVbqGSumvf52brhda/Code-Search?type=design&node-id=4176%3A87879&mode=design&t=DcvvxOTfX6VTepfM-1

  • Change the icon. I've added one for left panel collapse and one for left panel expand.
  • Group the revision selector and home button into one container with 4px extra padding to the left + right of it (as shown in the screenshot sbove)

Collapse left panel
CleanShot 2024-05-14 at 05 48 14@2x

Expand left panel
CleanShot 2024-05-14 at 05 48 35@2x

Thanks!

@taiyab
Copy link
Contributor

taiyab commented May 14, 2024

As a side ask from me too, can we always highlight the panel dividers/separators like this when you hover them?

CleanShot 2024-05-14 at 05 52 15@2x
  • Change to primary-01
  • Add a glow effect (probably same as input focus)
  • Transition the hover effects over 250ms (fade in/out)

Basically, like Slack.

@vovakulikov
Copy link
Contributor Author

vovakulikov commented May 14, 2024

@taiyab

  • I fixed the rowing problem and moved the root button in the first row (also fixed paddings to 0.5rem; in designs, it isn't consistent; the expand button and rev button have 16px, and the rev button and go to the root button have 12px.
  • I didn't change the icons, simply because we still use MDI icons and not the new icons, I filed an issue for this here Svelte: Sync icons with new set in Figma #62661
  • Agree about the resize panel change; I would do it in a follow-up about the resize panels UI here [Svelte]: Resizable panels UI improvements #62662

Copy link
Contributor

@fkling fkling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍🏻

client/web-sveltekit/src/lib/TreeNode.svelte Outdated Show resolved Hide resolved
client/web-sveltekit/src/lib/TreeNode.svelte Outdated Show resolved Hide resolved
Comment on lines 163 to 165
li[data-treeitem][aria-selected='true'] > & {
color: var(--tree-node-lable-color, var(--body-bg));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if that's better, but we could also set the color/variable in [role="treeitem"] (and &[aria-selected="true"]) and just use color: var(...) here, and avoid using this selector.
No preferences though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with setting variables or color on [role="treeitem"] level is that all children levels also get these variable or color values, which isn't correct when these styles are used for selected/hover states.

I did this at the very beginning too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though "resetting" it to the default color by a treeitem descendant would override that setting but maybe not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, CSS cascade doesn't work with recursive like this (if I understood your comment correctly), an interesting thing is that I wanted to have left padding for nesting also based on CSS variables but it turned out this expression doesn't work

:root {
   --nesting-level: 0;
 }
 
 .item-children {
     --nesting-level: calc(var(--nesting-level) + 1);
  }
  

Comment on lines +197 to +200
:global([data-tree-view-flat-list='true']) & {
width: 0;
margin-left: 0.5rem;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't quite understand why this is necessary until I opened the page and changed the styles dynamically.
I agree that not using as much horizontal space when it's not needed looks good, but I also noticed that the whole tree "shifts" when going up to a non-flat parent, which feels a bit weird. Overall I wonder if this edge case is worth having the extra rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I also noticed that the whole tree "shifts" when going up to a non-flat parent, which feels a bit weird

I have no preferences here, but I think the tree jumps regardless of the padding here; the current level gets additional paddings anyway as we open the parent level, no?

Comment on lines +32 to +38

/**
* Resets file tree top level path, in some cases like jump to scope
* we have to invalidate cache since top level path has been changed
* to a lower level.
*/
resetTopPathCache(repoName: string, revision: string): void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels very "manual". Maybe there is a better way to do this at some point, but I can't think of anything else atm either.

Copy link
Member

@camdencheek camdencheek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR seems to sometimes introduce a horizontally scrolling container, which is confusing because the content isn't actually horizontally scrollable except for a pixels in either direction

screenshot-2024-05-14_15-11-41.mp4

@camdencheek
Copy link
Member

Not quite sure what's going on, but when I run this branch locally, the fuzzy finder activates on every page load. This does not happen for me on main. Anyone else?

@vovakulikov
Copy link
Contributor Author

@camdencheek, this is my bad. One commit slipped when I was fixing it by Felix's comments. I just pushed it, it should be working now.

@vovakulikov
Copy link
Contributor Author

because the content isn't actually horizontally scrollable except for a pixels in either direction

I'll take a look; it's actually an interesting question: should this block be scrollable by a horizontal axis? It probably should (the proper scroll, not just a few pixels) if the tree has that nested structure like this. Otherwise it won't be possible to work with this tree only if they jump the scope with new control that this PR adds but this thing should be optional I guess

@taiyab
Copy link
Contributor

taiyab commented May 15, 2024

Excited for this to be merged in! Great work @vovakulikov.

@vovakulikov
Copy link
Contributor Author

@camdencheek I fixed the problem with width and undesirable small scroll, thanks for spotting this. For now I just hide overthing that falls outside of the sidebar width. This is somehting we have at the moment on the main so no regressions here, the problem would partially solved by new "move scope to dir" button but it's still possible to open some really nested strucuter and see no title of the selected item. For example on our test wierd repo

Screenshot 2024-05-15 at 14 40 52

@taiyab Curios what you think about making the sidebar scrollable by X axis and maybe make file items titles sticky by X axis as well. Making it scrollable would solve problem with overflowed nested items, making them (file items title) sticky by X axis would solve the problem when you will scroll to the right to see nested items, you also will still be able to see file items from the top level since they're sticky

but this also would me that if you make panel small enough we just cut file item titles (now they are truncated and you can see ... at the end)

@vovakulikov
Copy link
Contributor Author

I'm going to merge this one since this doesn't introduce any regressions in the current layout behavior but again I think the problem still remains

@vovakulikov vovakulikov merged commit cb7b38e into main May 15, 2024
12 checks passed
@vovakulikov vovakulikov deleted the vk/update-file-tree-panel branch May 15, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants