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

[DialogBox] : Improve display for remove folder from workspace #7449

Merged

Conversation

Anasshahidd21
Copy link
Contributor

@Anasshahidd21 Anasshahidd21 commented Mar 30, 2020

What it does

Fixes: #7443

Improves the display of remove from workspace dialog, adds codicon root-folder and gives some margin between different items in
the dialogBox for better visual impact.

Also updated the text, and added tooltips on both the folders and the textContent which now displays the path to the root and the note respectively.

Signed-off-by: Anas Shahid muhammad.shahid@ericsson.com

How to test

  1. Open Theia
  2. Open a workspace
  3. Add more folder(s) to the workspace
  4. Remove the folder(s) from workspace

Review checklist

Reminder for reviewers

@Anasshahidd21 Anasshahidd21 force-pushed the removeFolderWorkspace branch 2 times, most recently from f29b1a3 to 7fbff45 Compare March 31, 2020 03:35
@vince-fugnitto vince-fugnitto added dialogs issues related to dialogs ui/ux issues related to user interface / user experience workspace issues related to the workspace labels Mar 31, 2020
@Anasshahidd21 Anasshahidd21 force-pushed the removeFolderWorkspace branch 4 times, most recently from fca6f89 to 0e01163 Compare April 3, 2020 18:41
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 look much nicer than before 👍
I like that we are using the new root-folder codicon icon, and the alignment looks much better.

Screen Shot 2020-04-06 at 9 36 02 AM

Comment on lines 109 to 113
.theia-DialogNode{
line-height: var(--theia-content-line-height);
margin-top: calc(var(--theia-ui-padding)*1.5);
margin-left: calc(var(--theia-ui-padding)*2.5);
}
Copy link
Member

Choose a reason for hiding this comment

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

[minor] the styling of css classes can be improved for consistency and readability:

Suggested change
.theia-DialogNode{
line-height: var(--theia-content-line-height);
margin-top: calc(var(--theia-ui-padding)*1.5);
margin-left: calc(var(--theia-ui-padding)*2.5);
}
.theia-DialogNode {
line-height: var(--theia-content-line-height);
margin-top: calc(var(--theia-ui-padding)*1.5);
margin-left: calc(var(--theia-ui-padding)*2.5);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for your review @vince-fugnitto. I have updated the code with this change in mind.

@akosyakov
Copy link
Member

I like the usage of codeicons, more than dots 👍 Hope other know what a dot for the folder means.

Fixes: eclipse-theia#7443

Improves the display of remove from workspace dialog, adds ellipses
before the root-name and gives some margin between different items in
the dialogBox for better visual impact.

Signed-off-by: Anas Shahid <muhammad.shahid@ericsson.com>
@vince-fugnitto
Copy link
Member

@lmcbout would you like to try it out as well?

@lmcbout
Copy link
Contributor

lmcbout commented Apr 8, 2020

I will test this PR shortly

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

Works fine, looks fine
Question: In the issue description : " adds ellipses before the root-name", I could not see this in action . Do I need to do something to get it ?

@Anasshahidd21
Copy link
Contributor Author

Works fine, looks fine
Question: In the issue description : " adds ellipses before the root-name", I could not see this in action . Do I need to do something to get it ?

Thankoyou for approving @lmcbout, it was a mistake from my side, I have updated the description of the PR, previously I were to add ellipses to it, but since we have codicons It was best to add a codicon to it.

@vince-fugnitto vince-fugnitto merged commit 750d846 into eclipse-theia:master Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialogs issues related to dialogs ui/ux issues related to user interface / user experience workspace issues related to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DialogBox] : Improve display for remove folder from workspace
4 participants