-
Notifications
You must be signed in to change notification settings - Fork 49
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
Suffix ISFS folder name with web app path #1484
Suffix ISFS folder name with web app path #1484
Conversation
@@ -146,7 +146,7 @@ export async function addServerNamespaceToWorkspace(resource?: vscode.Uri): Prom | |||
const params = new URLSearchParams(uri.query); | |||
const project = params.get("project"); | |||
const csp = params.has("csp"); | |||
const name = `${project ? `${project} - ` : ""}${serverName}:${namespace}${csp ? " web files" : ""}${ | |||
const name = `${project ? `${project} - ` : ""}${serverName}:${namespace}${!csp ? "" : ["", "/"].includes(uri.path) ? " web files" : ` web files (${uri.path})`}${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the names shorter? How about server:NS (Web)
and server (app/path)
? We should change the project one to server:NS (project)
as well to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that better, but I think we can remove the namespace when we're tied to a specific web app. IMO that extra info isn't worth the space in that case. I see what you mean with the potential web and project conflict. Do you want to leave the "web files" suffix then? I doubt that conflict will come up in reality but it could be confusing for a user if it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I forgot that the project used to come first! I agree, let's put that back.
This PR fixes #1483
Before the change the result was: