Skip to content

Prompt for additional metadata during issue/pr create - #839

Merged
mislav merged 11 commits into
issue-pr-create-metadatafrom
issue-pr-create-metadata-wizard
May 8, 2020
Merged

mislav merged 11 commits into
issue-pr-create-metadatafrom
issue-pr-create-metadata-wizard

Conversation

@mislav

@mislav mislav commented Apr 27, 2020 •

Copy link
Copy Markdown
Contributor

Per discussion finalized in #768 (comment), this adds add an Add metadata prompt to issue create and pr create wizards.

Todo:

  • do not offer to add metadata in a repository where you don't have triage privileges
  • sort teams in the "reviewers" list alphabetically

Depends on #787
Closes #768, fixes #340

@mislav
mislav requested review from probablycorey and vilmibm April 27, 2020 17:05
@vilmibm vilmibm mentioned this pull request Apr 29, 2020
@mislav

mislav commented May 4, 2020 •

Copy link
Copy Markdown
Contributor Author

This is ready to be taken for a spin! To test this out:

  1. bin/gh issue create -R <OWNER/REPO>
  2. bin/gh pr create -R <OWNER/REPO>

Tips:

  • Choose a repo in a large organization to get a feel of how multiple selects work for many assignees/reviewers;
  • For testing, as long as you always choose "Cancel" in the end instead of "Submit" no branch will be published nor any data submitted.

My observations:

  • In my testing, I found it confusing that multi-select items are chosen with Space, while single-select lists are chosen with Enter. If users accidentally press the wrong key to confirm their selection and move on, they might get confused by unexpected results; in multi-select, Enter will move on to the next prompt even if no item was selected, and in single-select (e.g. milestone picker) Space will enter filtering mode and Backspace will be needed to recover from that. These key interactions are defined by the defaults of the Survey library for terminal prompts.
  • For a large org such as github, choosing to select assignees/reviewers/projects results in a large (~10s) delay with no visual feedback. I suggest that we display the loading spinner in the meantime to avoid having the command feel like it's "stuck"

/cc @ampinsk

@tierninho

Copy link
Copy Markdown
Contributor

I agree with those observations. I found it confusing at first, but eventually got used to it. I still wished there was a way to go back and edit but I suppose the user can select Continue in Browser and make the edits as needed.

  • I believe I spotted a bug when there is no Project set by the repo admin. It returned this and then ended the gh pr create abruptly without saving any of my selections:

    could not collect title and/or body: could not prompt: please provide options to select from

Screen Shot 2020-05-04 at 9 59 45 AM

  • Also, when the user opts to add metadeta, but then hits enter right away, we still show the "What would you like to add?" text but with no result after it. Not a big deal, but wanted to point it out. I guess no answer means "nothing" 🤷

Screen Shot 2020-05-04 at 9 56 15 AM

@probablycorey probablycorey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good and I tested it out and it worked great even on large repos!

Is it time to rename the titleBody struct now that it has so much other information? I realize this PR didn't introduce any changes in it, but I keep forgetting what that type represents now because it is so much more than titleBody now.

Comment thread command/title_body_survey.go Outdated
return err
}
inProgress.Body = templateContents
data.Body = templateContents

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It took me a second to figure out what data is used for. Is it a go idiom to use the name data for a var that returns a value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that data isn't the best variable name. Basically, data here is an instance of titleBody which by now basically serves as a state object during the wizard. I'll try to rename these variables and types so it's more clear what's going on.

Comment thread command/title_body_survey.go Outdated
}

confirmA, err := confirmSubmission(allowPreview)
confirmA, err := confirmSubmission(!data.HasMetadata(), true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The args here make it a little confusing. Maybe pull them out as individual vars instead of passing the args inline?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call!

@mislav

mislav commented May 5, 2020

Copy link
Copy Markdown
Contributor Author
  • I believe I spotted a bug when there is no Project set by the repo admin. It returned this and then ended the gh pr create abruptly without saving any of my selections:

@tierninho Nice find!

@ampinsk

ampinsk commented May 5, 2020

Copy link
Copy Markdown

Overall I think this is looking awesome!

In my testing, I found it confusing that multi-select items are chosen with Space, while single-select lists are chosen with Enter. If users accidentally press the wrong key to confirm their selection and move on, they might get confused by unexpected results; in multi-select, Enter will move on to the next prompt even if no item was selected, and in single-select (e.g. milestone picker) Space will enter filtering mode and Backspace will be needed to recover from that. These key interactions are defined by the defaults of the Survey library for terminal prompts.

Agreed this is a little awkward but I think under the circumstances and once you're used to it it's not too bad. Maybe we can update the instructions [Use arrows to move, space to select, type to filter] to include Enter to continue? That's getting long but maybe we could replace Use arrows to move since that one feels more obvious? Happy to leave this as an enhancement later and know we're probably constrained by the library here.

For a large org such as github, choosing to select assignees/reviewers/projects results in a large (~10s) delay with no visual feedback. I suggest that we display the loading spinner in the meantime to avoid having the command feel like it's "stuck"

+1 on using a spinner for these long waits!

@mislav
mislav merged commit 4a3588d into issue-pr-create-metadata May 8, 2020
@mislav
mislav deleted the issue-pr-create-metadata-wizard branch May 8, 2020 16:55
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.

5 participants