This repository has been archived by the owner on Jun 25, 2020. It is now read-only.
fix(legacy-table): avoid React DOM #392
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`jquery.datatables` will manipulate DOMs, sometimes remove them. In case of component being reloaded with updated props, React will not be able to find those removed nodes, causing a `cannot removeChild` error. Because of the the way to assign row keys, if table shape changes (add or remove columns), React may also have difficulty match the cached nodes via keys. In general it's a bad idea to directly manipulate React rendered DOM nodes, so we better just let `jquery.datatables` handle everything. In the future, once we removed `jquery.datatables`, a pure React component will not have such issues.
|
Deploy preview for superset-ui-plugins ready! Built with commit b7596d0 |
kristw
approved these changes
Mar 7, 2020
|
Can't wait to get rid of |
I don't think so. |
ktmud
force-pushed
the
react-legacy-table
branch
from
bb38eaf
to
0e95808
Compare
Codecov Report
@@ Coverage Diff @@
## master #392 +/- ##
========================================
+ Coverage 1.48% 2.78% +1.3%
========================================
Files 185 186 +1
Lines 5805 5812 +7
Branches 370 372 +2
========================================
+ Hits 86 162 +76
+ Misses 5707 5623 -84
- Partials 12 27 +15
Continue to review full report at Codecov.
|
kristw
reviewed
Mar 7, 2020
kristw
reviewed
Mar 9, 2020
packages/superset-ui-legacy-plugin-chart-table/test/test_data.ts
Outdated
Show resolved
Hide resolved
| import { mount } from 'enzyme'; | ||
| import ReactDataTable from '../src/ReactDataTable'; | ||
| import transformProps from '../src/transformProps'; | ||
| import * as testData from './test_data'; |
There was a problem hiding this comment.
Perhaps use specific import to enable warning if we later do not use some of these.
import { basic, advance } from ...
There was a problem hiding this comment.
Wanted to have a namespace so it's more explicit in later code, but also wanted to keep variable names simple.
There was a problem hiding this comment.
Added a default export for testData. I think it looks better than import *.
In the future, maybe all plugins can reuse the same test data---similar to switching visualization type in Superset.
packages/superset-ui-legacy-plugin-chart-table/test/ReactDataTable.test.tsx
Outdated
Show resolved
Hide resolved
ktmud
force-pushed
the
react-legacy-table
branch
from
d1d76c9
to
c8a58c9
Compare
Not seeing the console.warn errors anymore. So cleaning it up. Previously it was from `<SuperChart />` component, but since we have updated the test case to not use <SuperChart>, we are good now.
ktmud
added a commit
to ktmud/superset
that referenced
this pull request
Mar 10, 2020
This fixes bugs related to chart rerendering. apache-superset/superset-ui-plugins#392
1 task
kristw
pushed a commit
to apache/superset
that referenced
this pull request
Mar 10, 2020
This fixes bugs related to chart rerendering. apache-superset/superset-ui-plugins#392
nytai
pushed a commit
to preset-io/superset-ui-plugins
that referenced
this pull request
Apr 27, 2020
* fix(legacy-table): avoid React DOM `jquery.datatables` will manipulate DOMs, sometimes remove them. In case of component being reloaded with updated props, React will not be able to find those removed nodes, causing a `cannot removeChild` error. Because of the the way to assign row keys, if table shape changes (add or remove columns), React may also have difficulty match the cached nodes via keys. In general it's a bad idea to directly manipulate React rendered DOM nodes, so we better just let `jquery.datatables` handle everything. In the future, once we removed `jquery.datatables`, a pure React component will not have such issues. * fix(legacy-table): handle the case when percentMetrics is null * fix(legacy-table): linting errors * refactor: use nimbus build * test(legacy-table): add React component tests * test(legacy-table): more sophisticated cases * fix: address PR apache-superset#392 comments * chore(legacy-table): clean up tests case setups Not seeing the console.warn errors anymore. So cleaning it up. Previously it was from `<SuperChart />` component, but since we have updated the test case to not use <SuperChart>, we are good now. * fix(legacy-table): misleading comment
lexisstv
pushed a commit
to utrace-ltd/superset-ui-plugins
that referenced
this pull request
Jun 1, 2020
* fix(legacy-table): avoid React DOM `jquery.datatables` will manipulate DOMs, sometimes remove them. In case of component being reloaded with updated props, React will not be able to find those removed nodes, causing a `cannot removeChild` error. Because of the the way to assign row keys, if table shape changes (add or remove columns), React may also have difficulty match the cached nodes via keys. In general it's a bad idea to directly manipulate React rendered DOM nodes, so we better just let `jquery.datatables` handle everything. In the future, once we removed `jquery.datatables`, a pure React component will not have such issues. * fix(legacy-table): handle the case when percentMetrics is null * fix(legacy-table): linting errors * refactor: use nimbus build * test(legacy-table): add React component tests * test(legacy-table): more sophisticated cases * fix: address PR apache-superset#392 comments * chore(legacy-table): clean up tests case setups Not seeing the console.warn errors anymore. So cleaning it up. Previously it was from `<SuperChart />` component, but since we have updated the test case to not use <SuperChart>, we are good now. * fix(legacy-table): misleading comment
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
jquery.datatableswill manipulate DOMs, sometimes remove them. In case of component being reloaded with updated props, React will not be able to find those removed nodes, causing acannot removeChilderror.Because of the the way to assign row keys, if table shape changes (add or remove columns), React may also have difficulty match the cached nodes via keys.
In general it's a bad idea to directly manipulate React rendered DOM nodes, so we better just let
jquery.datatableshandle everything.In the future, once we removed
jquery.datatables, a pure React component will not have such issues.This PR also contains a fix to null
percent_metrics.cc @kristw @etr2460
🐛 Bug Fix