-
Notifications
You must be signed in to change notification settings - Fork 269
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
Interactive documentation for side panel #351
Conversation
package.json
Outdated
}, | ||
{ | ||
"view": "vscode-edge-devtools-view.targets", | ||
"contents": "If you have any questions or want to learn more, check the [docs on GitHub](https://github.com/microsoft/vscode-edge-devtools/blob/master/README.md)", |
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.
Will add a period at the end of this sentence when I go to address feedback
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.
Overall LGTM with a small request around the generated launch.json file (if it's even possible). It would also be good to get one of the others to review since I'm pretty new to this space
src/utils.ts
Outdated
const launchJson = vscode.workspace.getConfiguration('launch', workspaceUri); | ||
let configs = launchJson.get('configurations') as vscode.DebugConfiguration[]; | ||
let configWithInstruction = {...providedDebugConfig}; | ||
configWithInstruction.url += ' **Replace with your website url before launching**'; |
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.
It might be a cleaner experience if we instead default to the new startpage in the generated launch.json, and leave a comment after the url that contains this text. That way, the generated launch.json is valid without any edits?
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.
That would depend on #350 . Or we could default to the localhost link for now and I could update my PR if this lands first
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.
One comment about showing empty target list content right away.
package.json
Outdated
{ | ||
"view": "vscode-edge-devtools-view.targets", | ||
"contents": "Launch an instance of Microsoft Edge to begin inspecting and modifying webpages.\n[Launch Instance](command:vscode-edge-devtools-view.launch)", | ||
"when": "titleCommandsRegistered && launchJsonStatus != Supported" |
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.
We should show all the content for the empty target list immediately and not wait on the titleCommandsRegistered
flag. We can disable the buttons that actually launch commands until they're ready with the enablement
flag when the commands are registered.
…tools into user/flynnolivia/sidebar-cta fix merge
I think things got a little messed up from checking in the linting changes in the middle of working on this. I'm going to open a new branch and put my changes on there: #357 |
Let's close/abandon this PR if we no longer need it to avoid confusion. |
This PR adds explanatory welcome content to the side pane when there are no targets to display. Users are prompted to launch an instance of Edge and configure their launch.json file to streamline the workflow.
Here's how the "getting started" workflow looks with a project that originally has no launch.json:
Summary of the changes:
Closes #337