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

Show pull request id in title #540

Merged
merged 3 commits into from Oct 5, 2018

Conversation

justinliew
Copy link
Contributor

Addresses #537

@justinliew
Copy link
Contributor Author

@RMacfarlane what's the process for getting PRs looked at? Also I didn't get a CLA bot on this and I think I need to deal with that. Thanks!

@RMacfarlane
Copy link
Contributor

Hmm, I'm not sure why the bot didn't kick in. I'll look into it.

There's not really a formalized process for reviews, right now I'm trying to get to them within a day of their creation, but am also balancing other work 😃

@justinliew
Copy link
Contributor Author

Hmm, I'm not sure why the bot didn't kick in. I'll look into it.

There's not really a formalized process for reviews, right now I'm trying to get to them within a day of their creation, but am also balancing other work 😃

Ok - thanks for replying so quickly and for all your hard work! I'm new to the open source stuff but Hacktoberfest has really inspired me to try to find tools I use and help improve them :)

@RMacfarlane
Copy link
Contributor

Awesome!

I looked more into the CLA bot, documentation on it says it doesn't trigger on small changes. There's a checkmark next to your commit saying that all CLA requirement are met, so we're good there.

This does clearly fix the problem in #537 but it also takes up more horizontal space, which I'm not sure we want. @misolori @rebornix what do you think, is there another way we could show this info? We could show it in the tooltip, but that's not very useful at a glance. I wish the VSCode tree API allowed setting a description or providing a custom render function for tree items.

@miguelsolorio
Copy link
Contributor

Thanks for taking a look at this!

I do think that adding it to the front of the string adds visual noise when it's intended to be secondary info, so I think I prefer it being added to the end of the string instead:

image

Ideally if we could add decorations on the line, similar to the git/scm decorations, we could add a different color to make them stand out less, but I'm not sure if that's possible.

@justinliew
Copy link
Contributor Author

justinliew commented Oct 4, 2018

image
I think that's a good call; that way if stuff is cut off then it's the PR id, rather than part of the text. Plus, these are short PR ids; I'm sure with larger repos there are 4-5 digit ones which would definitely add unnecessary noise.

I've updated this PR.

@miguelsolorio
Copy link
Contributor

Thanks! I'd also double-check that it gets added to the tooltip, doesn't look like it was added there as well.

@justinliew
Copy link
Contributor Author

I just pushed a change to add the tooltip.

Copy link
Contributor

@miguelsolorio miguelsolorio left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for your help!

@RMacfarlane RMacfarlane merged commit 1180e06 into microsoft:master Oct 5, 2018
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.

None yet

3 participants