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 tooltips for sub-category folder #143

Merged
merged 4 commits into from
Feb 20, 2019

Conversation

pujiaxun
Copy link
Contributor

Background: I wonder how many problems I have solved. Then I found the tooltip field is available for now. That's it~

@jdneo
Copy link
Member

jdneo commented Feb 20, 2019

@pujiaxun Sounds great. Would you mind to attach a screenshot in the PR?

@pujiaxun
Copy link
Contributor Author

@pujiaxun Sounds great. Would you mind to attach a screenshot in the PR?

image

It looks like this. @jdneo

@jdneo
Copy link
Member

jdneo commented Feb 20, 2019

@pujiaxun Just one small comment.

@pujiaxun
Copy link
Contributor Author

        import * as os from "os";
        //...

        return [
            `AC: ${numAC}`,
            `Not AC: ${numNotAC}`,
            `Unknown: ${numUnknown}`,
            `Total: ${problems.length}`,
        ].join(os.EOL);

Like this? @jdneo

@jdneo
Copy link
Member

jdneo commented Feb 20, 2019

@pujiaxun Yes, looks much better.

@pujiaxun
Copy link
Contributor Author

@jdneo Any other suggestions? For instance, I think the name "Not AC" sounds weird...

@jdneo
Copy link
Member

jdneo commented Feb 20, 2019

@pujiaxun hmm, good question.

Regarding the wording. My suggestion is Not AC -> Failed

Either Unknown or Total, one of them can be removed I think? What do you think?

@pujiaxun
Copy link
Contributor Author

@jdneo I prefer to keep Total if I have to give up one of them. After all, the tag Unknown is somewhat confusing.

@jdneo
Copy link
Member

jdneo commented Feb 20, 2019

Same to me. So let's keep AC, Failed and Total.

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.

Just one more comment

src/explorer/LeetCodeTreeDataProvider.ts Outdated Show resolved Hide resolved
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.

@pujiaxun LGTM. Thank you for your contribution. Nice work.

@jdneo jdneo merged commit 72acef9 into LeetCode-OpenSource:master Feb 20, 2019
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