Move retry clone dialog into AppError #9944
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rafeca
suggested changes
Jun 8, 2020
| @@ -1,76 +0,0 @@ | |||
| import * as React from 'react' | |||
There was a problem hiding this comment.
So good to delete this file!!! 🌶️
| * Handle git clone errors to give chance to retry error. | ||
| * Doesn't handle auth errors. | ||
| */ | ||
| export async function gitCloneErrorHandler( |
Co-Authored-By: Rafael Oleza <rafeca@users.noreply.github.com>
Co-Authored-By: Rafael Oleza <rafeca@users.noreply.github.com>
Co-Authored-By: Rafael Oleza <rafeca@users.noreply.github.com>
Co-Authored-By: Rafael Oleza <rafeca@users.noreply.github.com>
Co-Authored-By: Rafael Oleza <rafeca@users.noreply.github.com>
…use in getDescriptionForError Co-Authored-By: Rafael Oleza <rafeca@users.noreply.github.com>
Co-Authored-By: Rafael Oleza <rafeca@users.noreply.github.com>
rafeca
previously approved these changes
Jun 9, 2020
Co-authored-by: Rafael Oleza <rafeca@github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
@rafeca ran into an edge case where a clone failed due to some refspec tomfoolery he was up to and noticed that the dialog presenting the error extended way beyond the window dimensions.
That's certainly not great UX. Upon seeing the image I was surprised that the git error output wasn't monospaced as I'd done some work to ensure that it was in #8964. Turns out that's because it's a custom dialog invoked through an error handler and as such doesn't enjoy any of the improvements we've made to app errors recently.
It also meant that the error text wasn't selectable, and that the error message didn't overflow using a capped max-height like in the AppError case.
What I've done now instead is to remove the custom error handler and move the detection logic for this particular error into the
AppErrordialog similar to how we've had custom support for auth errors there.Note: this does complicate the logic of the
AppErrorcomponent in a fairly nasty way since it means we'll have to check for this error in three places (when determining the title, when rendering the contents, and when rendering the footer). This is a limitation that me and @rafeca discussed and that we think we can get around with some refactoring of theAppErrorcomponent but not something I want to tackle in this PR.Screenshots
Before
After
Release notes
Notes: [Improved] Clearer presentation of error message when a clone fails