Folders from path #43659
Folders from path #43659
Conversation
- text-overflow handled by elliipsis appearing at the beginning of path
- adds word-wrap to inputBox messages - adds whether file or folder is going to be created
| /** | ||
| * Determines whether the ellipsis appears at the left or right side of the string | ||
| */ | ||
| export enum EllipsisType { |
There was a problem hiding this comment.
This does not seem to be needed anymore. Same as IShowMessageOptions
| @@ -392,6 +406,7 @@ export class InputBox extends Widget { | |||
| const styles = this.stylesForType(this.message.type); | |||
| spanElement.style.backgroundColor = styles.background ? styles.background.toString() : null; | |||
| spanElement.style.border = styles.border ? `1px solid ${styles.border}` : null; | |||
| spanElement.style.wordWrap = 'break-word'; | |||
There was a problem hiding this comment.
I would prefer if we added this in the inputBox.css here https://github.com/Microsoft/vscode/blob/isidorn/searchPanel/src/vs/base/browser/ui/inputbox/inputBox.css#L88
- removes unused interface and enum for ellipsis
| @@ -136,6 +136,33 @@ suite('FileService', () => { | |||
| }, error => onError(error, done)); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
@tsalinger I do not really understand why you added new tests in the fileService, when this feature does not touch anything inside the fileService as far as I understand
@bpasero can you please review if these tests are needed in the fileService
There was a problem hiding this comment.
These are exploratory tests that I wrote to test whether I can implement this feature the way I did.
| } | ||
| } | ||
|
|
||
| function replaceWithNativeSep(str: string) { |
There was a problem hiding this comment.
There should be a helper method in paths.ts so you do not need to introduce a new one. Try using what we alerady have.
| @@ -296,6 +296,7 @@ export class FileRenderer implements IRenderer { | |||
| }, 0); | |||
| }); | |||
|
|
|||
| const initialRelPath: string = relative(stat.root.resource.fsPath, stat.parent.resource.fsPath); | |||
There was a problem hiding this comment.
You only use this const further down. It is usually best to intiilize things exactly where you use them so they are scoped in the right place, please move it to the place where you actually use it.
| @@ -1335,35 +1336,77 @@ export function validateFileName(parent: IFileStat, name: string, allowOverwriti | |||
| return nls.localize('emptyFileNameError', "A file or folder name must be provided."); | |||
| } | |||
|
|
|||
| const names: string[] = trimTrailingSlashes(name) // prevents empty last array element after split | |||
There was a problem hiding this comment.
Remove trimTrailingSlashes ,just split go through the array and filetr out null elements. Like this
name.split(/[\\/]/).filter(part => !!part);
imho that is simpler
| return nls.localize('filePathTooLongError', "The name **{0}** results in a path that is too long. Please choose a shorter name.", trimLongName(name)); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function windowsMaxLengthRestrictionReached(name: string, parent: IFileStat): boolean { |
There was a problem hiding this comment.
I do not understand what is hte benefit of adding this function. It is the same functionality as before but now it is enapsulated in a function, but we still call the function only once.
Imho a best PR is when it contains the minimal changes need to make it happen, so I would remove this helper function
| return { exists: false, child: undefined }; | ||
| } | ||
|
|
||
| function compareFolderNames(name1: string, name2: string): boolean { |
There was a problem hiding this comment.
I do not really see the benefit of moving the folderName comparison to a seperate method. I would keep it as it iwas before, especially since compare functions usually return -1, 0 and 1 so for me this is confusing naming since it returns a boolean. If it returns a boolean it shoulld be areFolderNamesEqual. Though I prefer not having a method
| if (parent.children) { | ||
| let exists: boolean = parent.children.some((c) => { | ||
| let found = compareFolderNames(c.name, name); | ||
| if (found) { foundDupChild = c; } |
There was a problem hiding this comment.
We try to format our if statements that they body of the if is always in the new line, even if it is a single statement
| } | ||
|
|
||
| function alreadyExists(parent: IFileStat, name: string): { exists: boolean, child: IFileStat | undefined } { | ||
| let foundDupChild: IFileStat; |
There was a problem hiding this comment.
duplicateChild might be a better name, this way it sounds like a boolean
| @@ -316,6 +320,27 @@ export class FileRenderer implements IRenderer { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I feel like this should not be a function, either a lambda expression or a private method in the explorerViewer. Though if you prefer functions we can leave it this way
|
@tsalinger I have added some comments. Feel free to push, and just ping me here once you address what you feel like needs addressing. Nice work 🍻 |
- displayCurrentPath: file/folder not bold anymore
fixes #31305
fixes #9349