PR/Issue create flow tweaks by vilmibm · Pull Request #231 · cli/cli

vilmibm · 2020-01-15T18:12:12Z

Closes #70

This PR:

Changes the UI for opening an editor. Instead of just enter to open editor with no way to skip,
you now press e to open the editor and enter to skip the body prompt. This required bringing
in some of survey's code so it could be extended and tweaked (about 150 lines).
```
Creating pull request for create-flows into master in github/gh-cli

? Title cool pr
? Body (/usr/bin/vim) [e: launch editor][enter: skip for now] 
```
Tweaks the final options from Yes, Edit, and Cancel to Preview in Browser, Submit, and
Cancel. This means that if someone wants to just get to the browser but didn't specify --web
they can just enter through the prompts and then edit in the browser. It also means that even if
you like drafting in the terminal/editor you can continue to the browser to set labels and
assignees before submitting.
```
Creating pull request for create-flows into master in github/gh-cli

? Title cool pr
? Body (/usr/bin/vim) <Received>
? What's next?  [Use arrows to move, type to filter]
> Preview in browser
  Submit
  Cancel
```

A caveat is that if a user specifies --draft and then previews in browser, that info is lost. There is no way to default the PR create page to making a draft PR, unfortunately.

ampinsk · 2020-01-15T21:10:19Z

Woo this looks awesome! One tiny nit question:

Creating pull request for create-flows into master in github/gh-cli

? Title cool pr
? Body [e: launch vim][enter: skip for now]

Would it be possible to make this tweak above to the Body prompt? Instead of putting the editor in parenthesis, we could say Open in [default editor name]

vilmibm · 2020-01-15T22:59:33Z

@ampinsk absolutely! i actually totally meant to do this and just forgot.

mislav

The implementation looks great! I feel an overall improvement to how we handle survey prompts.

mislav · 2020-01-16T13:02:18Z

command/issue.go

+			baseRepo.RepoOwner(),
+			baseRepo.RepoName(),
+			url.QueryEscape(title),
+			url.QueryEscape(body),


You could potentially use url.Values to construct the query string so you don't have to implement the serialization yourself:

v := url.Values{} v.Set("title", title) v.Set("body", body) v.Encode() //=> "title=MYTITLE&body=HELLO"

this is good to know about but I don't really have a problem with the way it is now; it's more readable for me than using url.Values{}. I could see url.Values{} being great for a longer or more complicated set of query parameters, though.

mislav · 2020-01-16T13:03:04Z

command/issue.go

+			url.QueryEscape(title),
+			url.QueryEscape(body),
+		)
+		// TODO could exceed max url length for explorer


Good forward thinking!

Not much we can do in that case, I think. Let's wait to see if this becomes a problem.

mislav · 2020-01-16T13:03:50Z

command/issue.go

+			url.QueryEscape(body),
+		)
+		// TODO could exceed max url length for explorer
+		fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL)


Could we avoid printing the full URL and just print the portion without the query string, since the query string might become large?

I left the scheme out while I was at it since without the query string it's not a URL worth clicking on anyway.

mislav · 2020-01-16T13:06:55Z

command/pr_create.go

+			url.QueryEscape(title),
+			url.QueryEscape(body),
+		)
+		// TODO maybe do something about -d being discarded when previewing


I think it's fine to concede that some fields won't be possible to pass over query parameters. Since the user will be presented with the full UI to choose all this themselves, I don't think we need to support --draft (nor --labels, --assignees, etc. in the future) in this mode.

mislav · 2020-01-16T13:08:20Z

command/title_body_survey.go

-	_cancel      = iota
+	PreviewAction Action = iota
+	SubmitAction  Action = iota
+	CancelAction  Action = iota


Minor syntax nit: I think that due to how iota works, you can omit = iota from all but the first line?

you can but I really hate that about iota ;_; as it seems to be standard Go style though I'll suck it up and do it.

mislav · 2020-01-16T13:09:05Z

command/title_body_survey.go

 	"github.com/spf13/cobra"
 )

+type Action int


Nice use of type alias to improve communication!

mislav · 2020-01-16T13:10:32Z

command/title_body_survey.go

-			confirmed = true
-		case _unconfirmed:
-			continue
-		case _cancel:


Great job incrementally improving code style in this PR 🏆

mislav · 2020-01-16T13:40:56Z

command/editor.go

+		if err != nil {
+			return "", err
+		}
+		if r == 'e' {


I understand that you inlined this whole file just to be able to change a few lines such as this one. I'm guessing that Survey didn't support creating the kind of prompt that we wanted.

I wonder how we can communicate that most of the code in this file isn't written by us (so we avoid style-reviewing it), but point out the parts that we edited? My idea:

Extract this into its own package under pkg/ or internal/, since it doesn't seem like this technically needs to be in the command package; i.e. it doesn't access any of the other parts of command.

Add comments pointing out to parts that were edited.

Does that sounds reasonable?

i like this!

mislav · 2020-01-16T16:04:54Z

command/editor.go

+{{- else }}
+  {{- if and .Help (not .ShowHelp)}}{{color "cyan"}}[{{ .Config.HelpInput }} for help]{{color "reset"}} {{end}}
+  {{- if and .Default (not .HideDefault)}}{{color "white"}}({{.Default}}) {{color "reset"}}{{end}}
+	{{- color "cyan"}}[e: launch editor][enter: skip for now] {{color "reset"}}


Just noticed that this presentation doesn't match the design mockup: [E to launch editor, Enter to skip]

I feel that we should lean to being human-readable rather than separating mutually exclusive options in separate [...]; what do you think?

Perhaps we could also consider indicating that the E in E to launch editor is a key to be pressed? E.g. (e) or <e>

mislav · 2020-01-16T16:06:55Z

command/title_body_survey.go

 				Options: []string{
-					"Yes",
-					"Edit",
+					"Preview in browser",


Was it intentional that "Preview in browser" is the first (default) option rather than 2nd like in design mockups?

yeah, we talked about it out of band.

PR/Issue create flow tweaks

Relates to cli#231 Add the HookRegistry and a small subset of events (AgentInitializedEvent, StartRequestEvent, EndRequestEvent) as a POC for how hooks will work.

vilmibm added 6 commits January 14, 2020 15:20

towards extending survey.Editor behavior

5d15f14

e to edit body, enter to skip

fcf1c4a

note

2012b45

support previewing PRs in the browser

11aa009

support preview in browser for issue create

e41184a

use default to preserve non-interactive behavior

e2ea61d

vilmibm requested review from ampinsk, mislav and probablycorey January 15, 2020 18:12

vilmibm added 2 commits January 15, 2020 12:35

fix some TODOs

b31cd3f

use pluralize helper

110f98a

mislav approved these changes Jan 16, 2020

View reviewed changes

mislav reviewed Jan 16, 2020

View reviewed changes

vilmibm added 5 commits January 16, 2020 14:03

clean up body prompt text

a6e8621

hide potentially long query strings when printing urls

d81a5ec

move survey extension to its own package and clarify

2c92e2f

rely on iota syntax magic

4d0cc1d

make prompt match mockup more

4239784

vilmibm merged commit b30ea6b into master Jan 16, 2020

mislav mentioned this pull request Jan 22, 2020

Extract helper for URL display #243

Merged

mislav deleted the create-flows branch January 22, 2020 23:00

mislav pushed a commit that referenced this pull request Jan 23, 2020

Merge pull request #231 from github/create-flows

84b19a5

PR/Issue create flow tweaks

Conversation

vilmibm commented Jan 15, 2020

Uh oh!

ampinsk commented Jan 15, 2020

Uh oh!

vilmibm commented Jan 15, 2020

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mislav Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mislav Jan 16, 2020 •

edited

Loading