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

Migrate 404 page and init 500 page #12236

Merged
merged 3 commits into from Jan 24, 2022
Merged

Migrate 404 page and init 500 page #12236

merged 3 commits into from Jan 24, 2022

Conversation

HichamELBSI
Copy link
Contributor

All in the title

Screenshot 2022-01-18 at 17 58 27
Screenshot 2022-01-18 at 17 56 42

@@ -87,6 +88,7 @@ const Admin = () => {
<InstalledPluginsPage />
</Route>
<Route path="/404" component={NotFoundPage} />
<Route path="/500" component={InternalErrorPage} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ We init the 500 page although there is no redirection to /500 in the app for the moment

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's a good idea!

I think that at some point we should improve the error handling in the admin by having two different views:

  • one in production that redirects the user to the 500 page
  • one in development where we implement an error boundary properly

Anyway good work!

@codecov
Copy link

codecov bot commented Jan 18, 2022

Codecov Report

Merging #12236 (9da450e) into master (2f29c55) will not change coverage.
The diff coverage is n/a.

❗ Current head 9da450e differs from pull request most recent head 6bbdb97. Consider uploading reports for the commit 6bbdb97 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12236   +/-   ##
=======================================
  Coverage   47.68%   47.68%           
=======================================
  Files         212      212           
  Lines        8223     8223           
  Branches     1863     1863           
=======================================
  Hits         3921     3921           
  Misses       3547     3547           
  Partials      755      755           
Flag Coverage Δ
front ∅ <ø> (∅)
unit 47.68% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 740d6fa...6bbdb97. Read the comment docs.

@HichamELBSI HichamELBSI added issue: enhancement Issue suggesting an enhancement to an existing feature source: core:admin Source is core/admin package labels Jan 19, 2022
@HichamELBSI HichamELBSI added this to the 4.0.6 milestone Jan 19, 2022
soupette
soupette previously approved these changes Jan 24, 2022
Copy link
Contributor

@soupette soupette left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: HichamELBSI <elabbassih@gmail.com>
Signed-off-by: HichamELBSI <elabbassih@gmail.com>
Signed-off-by: HichamELBSI <elabbassih@gmail.com>
@HichamELBSI HichamELBSI merged commit b037974 into master Jan 24, 2022
@HichamELBSI HichamELBSI deleted the enh/404-500-pages branch January 24, 2022 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: enhancement Issue suggesting an enhancement to an existing feature source: core:admin Source is core/admin package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants