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 navigation items for issues and pull requests #99

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

csexton
Copy link

@csexton csexton commented Oct 13, 2018

GitHub added secondary navigation items for issues and pull requests. This extends the action to list the same items under each category.

Issues:

  • Created
  • Assigned
  • Mentioned

untitled

Pull Requests:

  • Created
  • Assigned
  • Mentioned
  • Review Requested

untitled 2

Renamed a few options:

  1. "My Open Issues" and "My Open Pull Requests" to match the top level navigation items in GitHub web UI. It is now "My Issues" and "My Pull Requests".

  2. "View All Issues" and "View All Pull Requests" to match the navigation items above. I the language of "View All Mentioned Pull Requests" or "View All Assigned Issues" seemed a little cumbersome.

Happy to change or update these name, just let me know. I was trying to be consistent with their web UI, but don't want to create confusion in the name of consistency.


By the way, thanks for building this action, I set out to make something similar and found this -- which handles way more than I every would have. I use it all the time now, it's fantastic.

GitHub has added secondary navigation items for issues and pull
requests. This extends the plugin to list these under each category.

Issues:
 - Created
 - Mentioned
 - Assigned

Pull Requests
 - Created
 - Mentioned
 - Assigned
 - Review Requested

Renamed a few options:

1. "My Open Issues" and "My Open Pull Requests" to match the
   top level navigation items in GitHub web UI. It is now "My Issues" and
   "My Pull Requests".

2. "View All Issues" and "View All Pull Requests" to match the
   navigation items above. I the language of "View All Mentioned Pull
   Requests" or "View All Assigned Issues" seemed a little cumbersome.
Update the generated code in a separate commit in case the maintainers
want to manage the changes separately.
Copy link
Owner

@bswinnerton bswinnerton left a comment

Choose a reason for hiding this comment

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

This looks great at first glance, @csexton! Thank you for your contribution.

I'll give this a more thorough review and cut a new release later today.

Copy link
Owner

@bswinnerton bswinnerton left a comment

Choose a reason for hiding this comment

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

I'll give this a more thorough review and cut a new release later today.

Narrator: he forgot. I'm so sorry about the delay on this, it fell to the bottom of my todo list.

👍 on renaming the values in defaultMenuItems, but as I pointed out below, I think that there's a bug that I just realized that we may not want to bring forward in your new suggestions. I'd like to hear your thoughts.

@@ -371,9 +371,24 @@ class GitHubLB {
} else {
return [
{
title: 'View All Pull Requests',
title: 'Created',
icon: 'pullRequestTemplate.png',
url: 'https://github.com/pulls',
Copy link
Owner

Choose a reason for hiding this comment

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

I just realized that this is a bug 🤔. The openAccountPullRequests function can be called for more users than just the current user. For example:

screen shot 2018-12-03 at 5 03 29 pm

Would go to https://github.com/pulls, which isn't quite right. I think that's something we should probably fix before adding these assigned, mentioned, and review requests filters to other accounts' openAccountPullRequests ability.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure I follow, so imma try to restate the problem and tell me if I am right.

If I enter a username of someone who is not me (say "bswinnerton"):

image

Then navigate to the pull requests and select "created":

image

It will take me to https://github.com/pulls for my account, not "bswinnerton".

untitled 2

BUT we expect it to take us to a page with the author, perhaps to a URL like:

https://github.com/pulls?utf8=✓&q=is%3Aopen+is%3Apr+author%3Abswinnerton+archived%3Afalse

untitled 3

Assuming this is correct understanding of the bug, it seems like two options:

  1. Construct the URL with the query params for the correct author
  2. Don't show these four options when the LaunchBar search is scoped an author or organization

Sorry if this is very verbose, but this rubber-duck process of writing this up helped me understand the bug.

Copy link
Owner

Choose a reason for hiding this comment

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

No need to apologize! Those screenshots are very helpful for the sake of posterity.

I am not sure I follow, so imma try to restate the problem and tell me if I am right.

Yes, precisely.

Assuming this is correct understanding of the bug, it seems like two options:

  1. Construct the URL with the query params for the correct author
  2. Don't show these four options when the LaunchBar search is scoped an author or organization

I agree with your assessment, and would be open to either approach if you're willing to push the PR towards one of these solutions. Personally I think it would be a nice user experience if we took option one 😄.

@@ -390,9 +405,19 @@ class GitHubLB {
} else {
return [
{
title: 'View All Issues',
title: 'Created',
icon: 'issueTemplate.png',
url: 'https://github.com/issues',
Copy link
Owner

Choose a reason for hiding this comment

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

Same thing here 😔 .

@csexton
Copy link
Author

csexton commented Jan 20, 2019

Turnabout is fair play, right? I am keeping things consistent with our 2 month development pace 🙃

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