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

fix(ui): retain user input when parsing invalid JSON during import #16255

Merged
merged 1 commit into from
Dec 18, 2019

Conversation

TCL735
Copy link
Contributor

@TCL735 TCL735 commented Dec 17, 2019

Closes #16099

Added a third party library that wraps JSON.parse with linting and detailed error messaging.

For now, we are going to put the detailed error messages into the toast. As @hoorayimhelping mentioned, there is a potential for moving the detailed error message into something more permanent and useful for the user.

Copy link
Contributor

@zoesteinkamp zoesteinkamp left a comment

Choose a reason for hiding this comment

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

Outside of my textarea comment everything looks fine to me

ui/cypress/e2e/dashboardsIndex.test.ts Outdated Show resolved Hide resolved
@TCL735 TCL735 force-pushed the fix_16099_retain_invalid_JSON branch 2 times, most recently from 6b4c1d3 to f2f1eea Compare December 18, 2019 00:40
@TCL735 TCL735 force-pushed the fix_16099_retain_invalid_JSON branch from f2f1eea to 7e3f0ac Compare December 18, 2019 01:20
Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

Great work, thanks for finishing this up! LGTM

  1. What are your thoughts on showing the error in the notification toast? When playing with this, I was thinking it might be better to show it in a more permanent and easy to read place. We might want to follow up with design about this, if so.

  2. The only other feedback I have is to say that the PR description is a great place to give some context into your thoughts and decisions. For example, I saw that you switched to a library instead of using JSON.parse. I assumed it's cause it gives better failure indications, but I don't know for sure. The PR description is a good place for you, the author to explicitly state anything so there aren't any assumptions made. It's also a good place to put testing instructions and / or gifs / screenshots.

Great work regardless!

@TCL735 TCL735 merged commit 409a438 into master Dec 18, 2019
@TCL735 TCL735 deleted the fix_16099_retain_invalid_JSON branch December 18, 2019 03:06
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.

Retain user input when JSON parsing fails
3 participants