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

Align "Add Folder" visibility with VSCode #10840

Conversation

colin-grant-work
Copy link
Contributor

What it does

This PR aligns the visibility of the context menu command to add a folder to a workspace with the corresponding command in VSCode.

How to test

On master, the command is visible if:

  • You right-click in empty space in the Navigator widget in an unsaved single-root workspace.
  • You right-click in on an existing root in the Navigator widget in a saved or multi-root workspace.

With these changes the command is visible if:

  • You right-click in empty space in the navigator widget in any workspace.

On master, there may not be any empty space in the Navigator widget. This PR adds a margin to a React virtualized scroll container to ensure that there is always some empty space at the bottom of the list, as there is in VSCode.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added navigator issues related to the navigator/explorer tree issues related to the tree (ex: tree widget) labels Mar 4, 2022
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work well for me 👍
I confirmed that:

  • the command is available from the more toolbar item
  • the command is available from the empty space in the explorer
  • the command from the command palette works well, and is properly enabled

Comment on lines +45 to +47
.theia-TreeContainer .ReactVirtualized__Grid__innerScrollContainer {
margin-bottom: calc(var(--theia-ui-padding) * 3);
}
Copy link
Member

Choose a reason for hiding this comment

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

I confirmed that the extra space works well, do we want to add it to all trees or just the explorer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should have it for all trees, since all trees have (by default, anyway) listeners for clicks targeting their nodes but no particular tree node, and that is only guaranteed to work if we ensure that all trees have some empty space. But I don't feel very strongly, since the Navigator is the only place I'm certain it matters.

Copy link
Member

Choose a reason for hiding this comment

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

I've only seen it being used in vscode for the explorer so I asked the question, but it would probably benefit all trees so we can keep it generic like you have it.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Confirming approval 👍

@colin-grant-work colin-grant-work merged commit 7700b56 into eclipse-theia:master Mar 4, 2022
@colin-grant-work colin-grant-work deleted the bugfix/add-folder-visibility branch March 4, 2022 15:49
@colin-grant-work colin-grant-work added this to the 1.24.0 milestone Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
navigator issues related to the navigator/explorer tree issues related to the tree (ex: tree widget)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants