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

chore(home): add ErrorBoundary to Charts section #12239

Merged
merged 2 commits into from
Jan 6, 2021

Conversation

maxamante
Copy link
Contributor

@maxamante maxamante commented Jan 4, 2021

SUMMARY

Hello, I am interested in contributing to Superset. Found issue #12149 and thought it would be a good way to learn the system. If you would like me to create a separate issue for this, I can do that.

#12149 wants to add an ErrorBoundary to specific components. I picked the ChartTable on the Welcome component, which I believe is the same as the 2nd item under the 5. Home Page mentioned in the issue. I replaced the root React.Fragment (<>) with an ErrorBoundary. I found the router logic and some other components using the ErrorBoundary this way.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

I wasn't sure how to proceed with testing or if it was desired. I didn't find any tests that tested the ErrorBoundary itself or any components that import it. The superset-ui repo has some tests I can take cues from. I'm more than happy to add tests, unless the ErrorBoundary is enough. Advice would be great!

ADDITIONAL INFORMATION

  • Has associated issue: Progresses Add ErrorBoundary to page components #12149 Fixes 5.2. Charts
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@rusackas rusackas closed this Jan 6, 2021
@rusackas rusackas reopened this Jan 6, 2021
@codecov-io
Copy link

codecov-io commented Jan 6, 2021

Codecov Report

Merging #12239 (fd22043) into master (f3ab1f4) will decrease coverage by 14.81%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #12239       +/-   ##
===========================================
- Coverage   66.20%   51.38%   -14.82%     
===========================================
  Files         996      466      -530     
  Lines       49174    16768    -32406     
  Branches     4993     4304      -689     
===========================================
- Hits        32554     8617    -23937     
+ Misses      16476     8151     -8325     
+ Partials      144        0      -144     
Flag Coverage Δ
cypress 51.38% <100.00%> (+7.28%) ⬆️
javascript ?
python ?

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

Impacted Files Coverage Δ
...set-frontend/src/views/CRUD/welcome/ChartTable.tsx 3.22% <ø> (-72.87%) ⬇️
...frontend/src/explore/components/DataTablesPane.tsx 58.46% <100.00%> (-4.56%) ⬇️
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx 11.76% <0.00%> (-88.24%) ⬇️
...src/dashboard/components/gridComponents/Header.jsx 10.52% <0.00%> (-86.85%) ⬇️
superset-frontend/src/components/IconTooltip.tsx 13.33% <0.00%> (-86.67%) ⬇️
...rc/dashboard/components/gridComponents/Divider.jsx 13.33% <0.00%> (-86.67%) ⬇️
... and 880 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 7cc0de1...fd22043. Read the comment docs.

@ktmud ktmud changed the title fix(ChartTable): replace root React.Fragment with ErrorBoundary chore(home): add ErrorBoundary to Charts section Jan 6, 2021
@ktmud
Copy link
Member

ktmud commented Jan 6, 2021

image

Tested by adding a undefined variable in the child component.

Doesn't look very nice as I think the page bottom needs more padding, but still better than a blank page or full page error message.

@ktmud ktmud merged commit 6df8224 into apache:master Jan 6, 2021
@ktmud
Copy link
Member

ktmud commented Jan 6, 2021

BTW, thanks for your first contribution, @maxamante ! Welcome aboard!

villebro pushed a commit to preset-io/superset that referenced this pull request Jan 7, 2021
villebro pushed a commit to preset-io/superset that referenced this pull request Jan 7, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants