Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Svelte: clean up file tree#63009

Merged
camdencheek merged 4 commits into
mainfrom
cc/clean-up-file-tree
Jun 4, 2024
Merged

Svelte: clean up file tree#63009
camdencheek merged 4 commits into
mainfrom
cc/clean-up-file-tree

Conversation

@camdencheek

@camdencheek camdencheek commented May 31, 2024

Copy link
Copy Markdown
Member

After the changes to the file tree layout in #62981, we can clean up the styles and DOM a bit. These changes are partially in prep for experimenting with alternative "current path" representations. Comments inline.

Test plan

Manual testing. There should be no change in behavior, just DOM/style simplification.

@cla-bot cla-bot Bot added the cla-signed label May 31, 2024
}

a {
flex: 1;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This shouldn't need to know it's in a flex box, so moved that to the TreeNode container

text-overflow: ellipsis;
overflow: hidden;
white-space: nowrap;
padding: 0.2rem 0.25rem 0.2rem 0;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This has weird padding because it adds padding to half of the label. Instead, we just add the padding on the label container so we don't need to keep it consistent across components.

bind:this={treeRoot}
on:keydown={handleKeydown}
on:click={handleClick}
data-tree-view-flat-list={isFlatList}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

data-tree-view-flat-list is no longer used because we no longer indent files more than directories

Comment on lines -130 to +124
--tree-node-left-padding: 1.25rem;
$shiftWidth: 1.25rem;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

$shiftWidth is a constant, so using an SCSS variable rather than a CSS variable.

@camdencheek camdencheek May 31, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also, with the guide lines, I think we could experiment with a more condensed display (by shrinking $shiftWidth) because the relationships are more obvious. (cc @taiyab if you're interested in experimentation)

Comment on lines -183 to -188
.scope-container {
position: absolute;
left: 0.2rem;
height: min-content;
display: flex;
}

@camdencheek camdencheek May 31, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Instead of making the scope button absolutely positioned and keeping track of the amount of space we need to reserve, we can just put it inline in the DOM and mark it visibility: hidden when it doesn't apply or isn't hover-activated.

Comment on lines +83 to +85
<Button variant="icon" on:click={handleScopeChange} data-scope-button>
<Icon svgPath={mdiImageFilterCenterFocusStrong} inline />
</Button>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Removed the wrapping div, allowing the button to sit directly in the DOM. Added data-scope-button so it's still able to be referred to in the CSS.

@camdencheek camdencheek force-pushed the cc/clean-up-file-tree branch from cee631a to 7c244d4 Compare May 31, 2024 18:03
@camdencheek camdencheek force-pushed the cc/clean-up-file-tree branch from 7c244d4 to b9647a9 Compare May 31, 2024 19:03
// but for some reason it's not recognized when using `svelte-check` and an error
// is thrown instead.
'data-testid'?: string
'data-scope-button'?: boolean

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm hopeful this will be fixed with native typescript support in Svelte 5

@camdencheek camdencheek marked this pull request as ready for review May 31, 2024 19:21
@camdencheek camdencheek requested a review from a team May 31, 2024 19:21
@camdencheek camdencheek merged commit a569959 into main Jun 4, 2024
@camdencheek camdencheek deleted the cc/clean-up-file-tree branch June 4, 2024 13:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants