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

Display 'path' when creating a new file/folder #6545

Merged
merged 1 commit into from
Nov 15, 2019
Merged

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Nov 13, 2019

What it does

Fixes #6544

  • displays the parent path when attempting to create a new file (path in which the file is created at).
  • displays the parent path when attempting to create a new folder (path in which the folder is created at).
  • when the URI selection from the explorer is at the workspace root display the entire path.
  • when the URI selection from the explorer is not the workspace root, display the relative path (ex: packages/core/src/common).
  • introduces the WorkspaceInputDialog dialog.
  • introduces the WorkspaceInputDialogProps which has a new parentUri prop

Example:

Workspace Root URI Selection

image

Non-workspace Root URI Selection

image

How to test

  1. verify the parent path is correct when opening the new file dialog (verify multiple locations)
    • parent path corresponds to the URI selection present in the explorer.
      • when selecting the workspace root or URI present in the root display the full path
      • when selecting non-workspace root, display the relative path
  2. verify the parent path is correct when opening the new folder dialog (verify multiple locations)
    • parent path corresponds to the URI selection present in the explorer.
      • when selecting the workspace root or URI present in the root display the full path
      • when selecting non-workspace root, display the relative path
  3. repeat for multi-root workspaces

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves ui/ux issues related to user interface / user experience labels Nov 13, 2019
@vince-fugnitto vince-fugnitto self-assigned this Nov 13, 2019
/**
* The general `path` to be displayed in the dialog.
*/
path: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do not we use path: Path (where Path is from @theia/core/core/lib/common/path)? No need to clarify the property with the "The general path" JSDoc which is not clear by the way.

@kittaakos
Copy link
Contributor

This is a nice to have UI/UX feature 👍

Dropping the input dialogs and doing what VS Code does when creating a new file/folder would be better in the long run.

@kittaakos
Copy link
Contributor

Example:

This behaves differently on Windows. Based on the screenshot you have attached, I should see the full FS path, but I see a project relative path on Windows.

screencast 2019-11-14 11-37-58

What it does

Fixes #6544

  • displays path when attempting to create a new file.
  • displays path when attempting to create a new folder.

Also, it is not clear what path means.

Fixes #6544

- displays the parent `path` when attempting to create a new file (path in which the file is created at).
- displays the parent `path` when attempting to create a new folder (path in which the folder is created at).
- when the URI selection from the explorer is at the workspace root display the entire path.
- when the URI selection from the explorer is not the workspace root, display the relative path (ex: packages/core/src/common).
- introduces the `WorkspaceInputDialog` dialog.
- introduces the `WorkspaceInputDialogProps` which has a new `parentUri` prop.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto
Copy link
Member Author

@kittaakos thank you for the review, I adjusted the code and documentation (including the PR description) to be more explicit. Please let me know if you have additional feedback.

@kittaakos
Copy link
Contributor

additional feedback.

I have tried it; it works as described 👍

Why do we distinguish between the root and other folders? I am wondering why don't we always show WS-root relative paths? Smells like a hack in the label provider.

@vince-fugnitto
Copy link
Member Author

additional feedback.

I have tried it; it works as described

Why do we distinguish between the root and other folders? I am wondering why don't we always show WS-root relative paths? Smells like a hack in the label provider.

Right, I'm not doing any special logic with creating the label besides using the labelProvider.
Perhaps you are right and that it should be properly updated.

@kittaakos kittaakos self-requested a review November 14, 2019 13:09
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I have verified it on Windows with the browser example; it works as expected 👍

@kittaakos
Copy link
Contributor

Why do we distinguish between the root and other folders? I am wondering why don't we always show WS-root relative paths? Smells like a hack in the label provider.

Follow-up: #6549

@vince-fugnitto
Copy link
Member Author

I have verified it on Windows with the browser example; it works as expected

Thank you for the review !

@vince-fugnitto vince-fugnitto merged commit d2ac015 into master Nov 15, 2019
@vince-fugnitto vince-fugnitto deleted the vf/GH-6544 branch November 15, 2019 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

display path to a new file in create file/folder dialog
2 participants