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

#2856 Improve ContextError display in Page Editor output panel #2860

Merged
merged 5 commits into from Mar 7, 2022

Conversation

BALEHOK
Copy link
Contributor

@BALEHOK BALEHOK commented Mar 2, 2022

Closes #2856

ErrorDetails components are reused. The logic in ErrorDisplay is very similar to EntryRow and can be moved to a helper/utility function.

InputValidationError:
image

ClientNetworkError:
Screen Shot 2022-03-02 at 9 52 51 PM

Template error (or any other error with the name property):
image

Logs tab:
image

image

TODO:

  • Add a title to InputValidationError (eg. "Input validation error")
  • Do not duplicate logic in ErrorDisplay and EntryRow

@BALEHOK BALEHOK requested review from twschiller and BLoe March 2, 2022 21:09
@BALEHOK BALEHOK force-pushed the feature/2856-error-display branch from 710bdcd to f5513cf Compare March 4, 2022 11:03
@BALEHOK BALEHOK force-pushed the feature/2856-error-display branch from a407006 to f7f542c Compare March 4, 2022 16:27
return <NetworkErrorDetail error={rootCause.error} />;
}

return entry.error.stack;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need the stack trace for unknown errors?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the only people who would understand the stack trace are probably capable of finding the stack trace in the browser console, right? Maybe we could show the first line of the stack trace or something? I'm just not sure when this would be useful to most page editor users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I don't think that the first line will be of any help.
Agree with the rest of your comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so should we remove this and just return a generic "unknown error occurred, please try again" type of message instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stack trace is removed, in case of "unknown" error it will show the name and message, see https://github.com/pixiebrix/pixiebrix-extension/pull/2860/files#diff-c412177eff772c4bca878f3bc805c720cbd34904f49e54cae4c8bc21fb850a5cR72

@BALEHOK BALEHOK marked this pull request as ready for review March 4, 2022 16:35
@twschiller twschiller added this to the 1.5.7 milestone Mar 4, 2022
@BALEHOK BALEHOK self-assigned this Mar 6, 2022
@BALEHOK BALEHOK changed the title Feature/2856 error display #2856 Improve ContextError display in Page Editor output panel Mar 6, 2022
@BALEHOK BALEHOK merged commit 3173b26 into main Mar 7, 2022
@BALEHOK BALEHOK deleted the feature/2856-error-display branch March 7, 2022 16:50
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.

Improve ContextError display in Page Editor output panel
3 participants