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

Add Click the tree explorer should open the topic (#132) #186

Merged
merged 8 commits into from
Mar 12, 2019

Conversation

xuzaixian
Copy link
Contributor

@xuzaixian xuzaixian commented Mar 5, 2019

Resolve #131

@jdneo
Copy link
Member

jdneo commented Mar 5, 2019

@xuzaixian Thank you for contribution.

From my understanding, after applying this change, when the user clicks on the tree node, we will generate new / open existing problem file, right?

@xuzaixian
Copy link
Contributor Author

yes ~

@jdneo
Copy link
Member

jdneo commented Mar 5, 2019

Emm, I'm ok to open the existing problem file.

While for the non-existing file, it will be better if we can first show the problem description to the user, and generate the file if the user has some further confirmation action. What do you think?

@xuzaixian
Copy link
Contributor Author

Hum....maybe we can preview the problem within a webview , and generate the file after confirmation action ?

@jdneo
Copy link
Member

jdneo commented Mar 6, 2019

@xuzaixian Exactly what I'm thinking about. And this action can be bound with a new command called Preview Problem.

Would you like to have a try to implement this feature first?

@xuzaixian
Copy link
Contributor Author

ok, i will work on it.

@jdneo
Copy link
Member

jdneo commented Mar 6, 2019

@xuzaixian I'm thinking that if we should create a new command or just reuse Show Problem

@xuzaixian
Copy link
Contributor Author

I prefer to create a new command Preview Problem .

use 'Show Problem' when they want to code right now
use 'Preview Problem' when they want to read description.
we can provide more choices to user 🤣

@jdneo
Copy link
Member

jdneo commented Mar 6, 2019

@xuzaixian So what should we do when user left-click on the node?

@xuzaixian
Copy link
Contributor Author

@jdneo How about just preview the problem description when user left-click?
We can provide an extra button called Code Now or something else .. in preview window,
The user can click button to code right away.
Also provide a new menu option in right-click menu called Code Now.

This sounds convenient.

@jdneo
Copy link
Member

jdneo commented Mar 6, 2019

@xuzaixian Yes, sounds good

@jdneo
Copy link
Member

jdneo commented Mar 6, 2019

Hi @xuzaixian,

I noticed that a lot of diffs is caused by reformating the package.json. Would you mind to revert those formatting changes? This could make it easier to review.

Thanks!

@jdneo jdneo self-requested a review March 6, 2019 10:48
@xuzaixian
Copy link
Contributor Author

sorry for that 😢

package.json Outdated Show resolved Hide resolved
src/commands/show.ts Outdated Show resolved Hide resolved
src/explorer/LeetCodeNode.ts Show resolved Hide resolved
src/shared.ts Outdated Show resolved Hide resolved
src/utils/webviewUtils.ts Outdated Show resolved Hide resolved
src/utils/webviewUtils.ts Outdated Show resolved Hide resolved
src/explorer/LeetCodeNode.ts Outdated Show resolved Hide resolved
Remove unused function
Use rem instead of px
Use the VS Code embed CSS vars in Preivew Webview
Use spaces instead of tabs in package.json
Use previewProvider to manage preivew logic
@jdneo
Copy link
Member

jdneo commented Mar 8, 2019

@xuzaixian You can install an extension called: TS Lint, to help you resolve those lint errors.

https://marketplace.visualstudio.com/items?itemName=ms-vscode.vscode-typescript-tslint-plugin

@xuzaixian
Copy link
Contributor Author

@jdneo
There is no error in my environment when i run vsce package
But The Travis CI build failed.
Can you take a look ?

@jdneo
Copy link
Member

jdneo commented Mar 8, 2019

@xuzaixian It's because the file system on Linux is case sensitive.

your file name is leetcodePreviewProvider.ts, but when you import it, you are using leetCode.

src/utils/webviewUtils.ts Outdated Show resolved Hide resolved
src/utils/webviewUtils.ts Outdated Show resolved Hide resolved
src/shared.ts Outdated Show resolved Hide resolved
Move the logic in webviewUtils.ts to leetCodePreviewProvider
Fix missing semicolon
Rename leetcodePreviewProvider to leetCodePreviewProvider
Copy link
Member

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @xuzaixian for the great work!

@jdneo jdneo merged commit f77737d into LeetCode-OpenSource:master Mar 12, 2019
@Vigilans Vigilans mentioned this pull request Mar 13, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants