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

feat: add ErrorBox showing full error details #4

Merged
merged 5 commits into from
May 24, 2020

Conversation

timdeschryver
Copy link
Member

When there's an error while running a query, the error isn't visible as the text will be "undefined".
Because of this, the error will never be shown at:

<div className="output">
  <span className="text-blue-600">&gt; </span>
  {parsed.text || parsed.error || 'undefined'}
</div>

Before
image

After
image

@timdeschryver
Copy link
Member Author

timdeschryver commented May 23, 2020

While taking the screenshot, I also saw the preview with "That is great, ship it!"
As this is not that great 😅 - I also changed that.

WDYT about the message, should it say something else?
We could later on add suggestions to resolve errors.

image

@timdeschryver timdeschryver force-pushed the pr/show-errors branch 2 times, most recently from 6ad9ec2 to c183788 Compare May 23, 2020 18:34
@smeijer
Copy link
Member

smeijer commented May 23, 2020

Thanks! This is awesome.

About the error, let's merge this fix. But at the same time, I think we should create an issue with a feature request. It should be possible to navigate over multiple results. But first things first, fix the bug 😇

Regarding the message, I do feel that showing the exact same error twice (under and next to the js code) is a bit weird.

If you take a look here:

https://github.com/smeijer/testing-playground/blob/e7634daec7b13c4942110341bea6548fdf5ca336/src/parser.js#L112-L113

You can see that we have both an error and an errorBody property. I think it would make sense to show the error + errorBody on the right when an error occurs? Only the error below, and both right next to it?

Hide the element info, and only show a big red error box on the right side with all the details?

What do you think?

@timdeschryver
Copy link
Member Author

I agree, I also found it weird looking - thanks for the suggestion!
If I understood it correctly, this is what you mean?

image

Also, if there's a better way to hide the element info feel free to ask for changes (or refactor it yourself if that's easier) as I'm not a React developer by day 😅

@smeijer
Copy link
Member

smeijer commented May 23, 2020

I haven't looked at the code yet, I'll do that tomorrow 😇

Regarding layout, that's exactly what I meant.

It can benefit from className="whitespace-pre-wrap", and the "divider" should be removed. (You also might need to merge in master, to get the errorBody as a string instead of an array)

image

@smeijer
Copy link
Member

smeijer commented May 24, 2020

I hope you don't mind. I've merged in master, and extracted the error handling to a dedicated ErrorBox component. I believe we can merge this thing 😎

What do you think?

left production, right this PR

image

@timdeschryver
Copy link
Member Author

@smeijer LGTM - the UI and the code are better than before 👍

@smeijer smeijer changed the title fix: show errors in output feat: add ErrorBox showing full error details May 24, 2020
@smeijer smeijer merged commit 129baea into testing-library:master May 24, 2020
@smeijer
Copy link
Member

smeijer commented May 24, 2020

Thanks for your work on this one @timdeschryver!

Now let's test this thing:

@all-contributors please add @timdeschryver for code

@allcontributors
Copy link
Contributor

@smeijer

I've put up a pull request to add @timdeschryver! 🎉

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.

2 participants