Skip to content
This repository was archived by the owner on May 24, 2024. It is now read-only.

Default error message #32

Merged
merged 9 commits into from
Jun 15, 2020
Merged

Default error message #32

merged 9 commits into from
Jun 15, 2020

Conversation

jmsv6d
Copy link

@jmsv6d jmsv6d commented Apr 22, 2020

Summary

Adding a default error message for the error boundary when there is none provided

Deployment Link

https://terra-applic-default-er-qucogs.herokuapp.com/tests/terra-application/application-error-boundary/error-boundary-test

Testing

Additional Details

Thank you for contributing to Terra.
@cerner/terra

@jmsv6d jmsv6d self-assigned this Apr 22, 2020
@mjhenkes mjhenkes temporarily deployed to terra-applic-default-er-qucogs April 22, 2020 14:19 Inactive
@mjhenkes mjhenkes temporarily deployed to terra-applic-default-er-qucogs April 27, 2020 15:15 Inactive
@@ -4,5 +4,6 @@
"terraApplication.unsavedChangesPrompt.multiplePromptMessageIntro": "There are unsaved changes made to the following:",
"terraApplication.unsavedChangesPrompt.multiplePromptMessageOutro": "Changes will be lost if you don't save them. How do you want to proceed?",
"terraApplication.unsavedChangesPrompt.acceptButtonText": "Don't Save",
"terraApplication.unsavedChangesPrompt.rejectButtonText": "Continue Editing"
"terraApplication.unsavedChangesPrompt.rejectButtonText": "Continue Editing",
"terraApplication.errorBoundary.defaultErrorMessage": "We are currently unable to complete your request. Try again later, and contact your support team if this problem continues."
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure these error boundary failures are the types of failures that will just work if you try again. Something massively went wrong in your application.

return (
<StatusView
variant="error"
message={activeError.toString()}
message={errorText}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could I get some more information about why we're changing the error boundary text? What cases are they being used for? I assumed they would be here in the case of major app failures. Are people seeing these a lot?

Copy link
Author

Choose a reason for hiding this comment

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

If there's an error thrown without any error message, then the error view will display the error type without any indication to the user about what went wrong. This would provide some sort of information to the user in that case. This would likely only happen if a custom error was thrown that didn't provide any message and was not caught anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have we seen instances where the thrown js exception doesn't have an error message? I don't mean to be difficult but I wondering what brought this change up?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mjhenkes, UX didn't like it. Didn't think it was user friendly

@jenniferwurth
Copy link

@ryanthemanuel: Discussed with the UXFoundations group, and there are potentially two versions depending on the use case I'll detail out below. Our traditional messaging when errors are present is to state what the issue is and how to resolve it. Typically and whenever possible, we would offer actions to the user to retry, log a ticket, or when no action is available to contact their system admin/IT support. Based on my conversations with Ryan this situation will only occur in non-prod environments where only an engineer or developer would encounter them, not a user (ie: clinician, revcycle staff, etc.) If this is the case where the user would know by using the error tracing what to correct. Our proposal is outlined below. please let me know if any assumptions are incorrect.

Scenario 1: Error messaging is short & concise, less than 2 lines of text.

  • Status View Type: Error
  • Icon: Error
  • Title: Error (centered under error icon)
  • Message: The system encountered an error: [insert error string here]. (left-aligned)

image

Scenario 2: Error messaging is long & has lots of coding jargon.

  • Status View Type: Error
  • Icon: Error
  • Title: Error (centered under error icon)
  • Message: The system encountered an error. (center-aligned)
  • Display Show/Hide component centered below messaging. "Show [error log/string/tracking]" (or whatever appropriate description of what will be shown when selected.
  • Display full error string when the user selects "Show [error string/log/tracking]". Text should be left aligned. When error string is displayed, "Show [error log/string/tracking]" is replaced with "Hide"

image

cc: @neilpfeiffer @jeremyfuksa

@ryanthemanuel
Copy link
Contributor

@jmsv6d let's go with scenario 1 here for now. I've logged cerner/terra-core#3000 to let us explore doing something more along the lines of scenario 2 with the status view.

@jeremyfuksa
Copy link
Contributor

Since you added the new issue, can we remove the UX review label on this one?

@mjhenkes mjhenkes temporarily deployed to terra-applic-default-er-mrykok June 11, 2020 14:14 Inactive
@mjhenkes mjhenkes temporarily deployed to terra-applic-default-er-mrykok June 12, 2020 20:21 Inactive
# Conflicts:
#	CONTRIBUTORS.md
#	src/application-error-boundary/ApplicationErrorBoundary.jsx
@mjhenkes mjhenkes temporarily deployed to terra-applic-default-er-mrykok June 15, 2020 14:29 Inactive
@mjhenkes mjhenkes temporarily deployed to terra-applic-default-er-mrykok June 15, 2020 16:44 Inactive
@jmsv6d jmsv6d merged commit 1b267ad into master Jun 15, 2020
@jmsv6d jmsv6d deleted the default-error-message branch June 15, 2020 17:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants