Create PR interstitial dialog refinements #2280
Conversation
We get this information through TypeScript, if we have them in comments the likelihood of us diverging and having conflicting information is high, better to have the one source of truth for type information.
niik
added
the
ready-for-review
| * that will be pushed to the user. If only one commit is to be pushed | ||
| * we return the singular 'commit', if any other amount of commits | ||
| * are to be pushed we return the plural 'commits'. If the | ||
| * capitalize parameter is true we'll capitalize the 'c' in commit |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| return ( | ||
| <ButtonGroup destructive={true}> | ||
| <Button type="submit">Cancel</Button> | ||
| <Button onClick={this.onPushOrPublishButtonClick}>Yes</Button> |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| <Button type="submit" onClick={this.onCreateWithoutPushButtonClick}> | ||
| No | ||
| </Button> | ||
| <Button onClick={this.onPushOrPublishButtonClick}>Yes</Button> |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| * it appears in the general style is an inline-box with a suitable background | ||
| * color, using a fixed-width font. | ||
| */ | ||
| export class Ref extends React.Component<{}, {}> { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
app/src/ui/lib/ref.tsx
Outdated
|
|
||
| /** | ||
| * A simple style component used to mark up arbitrary references such as | ||
| * branches, commit shas, paths or other content which needs to be presented |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
app/styles/ui/_ref.scss
Outdated
| @@ -0,0 +1,8 @@ | |||
| em.ref { | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
I tried out a new approach for the buttons. What I'm trying to achieve here that I think is important is that we ask an answerable question in these types of dialogs.
@joshaber how do you feel about that? Note that I haven't applied title casing here, I wanted to see if it's an acceptable approach on macOS first |
|
It's better than the plain Yes/No, though:
I agree, but conventionally these answers don't actually say "yes" or "no" on Mac, at least. For example:
|
|
Alright, I want to get this in before my vacation so I'll just remove the Yes, No completely but I would like to look at platform use of Updated images with the dialog refinements from #2277 brought in
|
I started out with the intention of just adding progress indication to the create PR interstitial dialog but got a bit carried away and did some refactoring, cleanup and style changes along the way.
Before
After
The dialog title as well as the content are rewritten to form an answerable question. The branch name has been emphasized using the new
Refcomponent instead of<b>which matches what we use elsewhere (we'll gradually move other custom styles over to this new component). I've also broken up the text a bit for readability.Here I've simply wired up the completion of the dispatcher promise to the
Dialog'sloadingprop which disables all buttons and adds the spinner up top.Finally I made the same text changes in the
aheadview of the dialog to have the text form an answerable question. Note that theCancelbutton have changed here toNo, meaning that if you clickNohere we won't push the branch but will still open the browser. Conceivably we could make this aYesNoCanceldialog but since theCreate PRaction is a non-destructive one (just opening a web page) I didn't feel that was warranted.Fixes #2265
Sidenote, @donokuda: I wonder if we should play around with a dedicated question dialog in the same vein as #2277 with a question octicon offsetting the content. Might be worth thinking about. With it we could enforce (albeit only by documentation) that such dialogs should pose an answerable question.