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

Handle PB Element plugin errors during render #726

Merged
merged 6 commits into from
Feb 25, 2020
Merged

Conversation

Fsalker
Copy link
Contributor

@Fsalker Fsalker commented Feb 24, 2020

Related Issue

#686

Your solution

Wrapped the two Element.tsx components inside Error Boundaries.

How Has This Been Tested?

The Render Element was tested by opening up the Pages. Here we clearly see an Element component which I've tested by intentionally throwing an error inside the Error Boundary.

image

Copy link
Collaborator

@Pavel910 Pavel910 left a comment

Choose a reason for hiding this comment

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

Added a few change requests :)

@Pavel910
Copy link
Collaborator

Can you also attach an image to the PR of what an error looks like when rendered? Thanks!

@Fsalker
Copy link
Contributor Author

Fsalker commented Feb 24, 2020

Here we go, there's an image now

@Fsalker
Copy link
Contributor Author

Fsalker commented Feb 25, 2020

Also, I'll remove the commented lines currently present in the code if this version of the PR is ok. Those lines help me test it out 😄

@Pavel910 Pavel910 changed the title fix: 'Element' components are enclosed in Error Boundaries now Handle PB Element plugin errors during render Feb 25, 2020
@Pavel910
Copy link
Collaborator

Pavel910 commented Feb 25, 2020

@Fsalker this looks ok, we can tweak minor things later on. One last thing: replace the An error has occured text with a simple Error: ${error.message}. That long text doesn't bring any meaning but takes up a lot of space. And remove the test errors of course ;)

@Fsalker
Copy link
Contributor Author

Fsalker commented Feb 25, 2020

alright, here we go. everything should be k now

Copy link
Collaborator

@Pavel910 Pavel910 left a comment

Choose a reason for hiding this comment

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

Let's merge this one and I'll give it another test before publishing.

@Pavel910 Pavel910 merged commit 43fec78 into webiny:master Feb 25, 2020
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