You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/07/30 07:27:09 UTC
[GitHub] [incubator-superset] ktmud opened a new pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
ktmud opened a new pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475
### SUMMARY
A more robust fix for #10349 and #10432 .
This should fix all similar problems related using DOM dimensions in charts before dashboard tab switch completes.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
#### Before
Table chart was not able to expand height when applying filters gave it more rows:
#### After
Table chart correctly expand height:
### TEST PLAN
Manual testing
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on a change in pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on a change in pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#discussion_r463183889
##########
File path: superset-frontend/src/chart/ChartContainer.jsx
##########
@@ -23,6 +23,11 @@ import * as actions from './chartAction';
import { logEvent } from '../logger/actions';
import Chart from './Chart';
+function mapStateToProps({ dashboardState }) {
+ // needed to prevent chart from rendering while animating
+ return { isParentMounted: dashboardState?.isTabMounted || true };
Review comment:
Won't this always be `true`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666845599
I still cannot reproduce. That was the reason for the bug before, but `theadHeight` shouldn't be 0 after the fix.
Can you try [changing this line](https://github.com/apache/incubator-superset/pull/10475/files#diff-97880ca268ecda8d41fbcdd21f1e6ddeR270) to `onEntered` and see if it fixes the bug for you?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666750570
It still seems to have that issue even with this latest change, i.e. the render height is set by the first render and not updated if the filter changes to include more rows
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-665895748
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=h1) Report
> Merging [#10475](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7a329c25e9867636fd8a79c3f19b45186bd0f95a&el=desc) will **decrease** coverage by `11.12%`.
> The diff coverage is `7.14%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10475 +/- ##
===========================================
- Coverage 70.80% 59.67% -11.13%
===========================================
Files 604 408 -196
Lines 32426 13350 -19076
Branches 3414 3415 +1
===========================================
- Hits 22960 7967 -14993
+ Misses 9354 5195 -4159
- Partials 112 188 +76
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `59.67% <7.14%> (-0.05%)` | :arrow_down: |
| #python | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `9.09% <ø> (-49.10%)` | :arrow_down: |
| [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `38.66% <0.00%> (-37.01%)` | :arrow_down: |
| [...d/src/dashboard/components/gridComponents/Tabs.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL1RhYnMuanN4) | `67.53% <0.00%> (-7.82%)` | :arrow_down: |
| [...nd/src/dashboard/containers/DashboardComponent.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZENvbXBvbmVudC5qc3g=) | `92.00% <ø> (-8.00%)` | :arrow_down: |
| [...-frontend/src/dashboard/reducers/dashboardState.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9yZWR1Y2Vycy9kYXNoYm9hcmRTdGF0ZS5qcw==) | `73.68% <0.00%> (-8.14%)` | :arrow_down: |
| [...t-frontend/src/dashboard/actions/dashboardState.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2Rhc2hib2FyZFN0YXRlLmpz) | `30.71% <33.33%> (-27.29%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [344 more](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=footer). Last update [7a329c2...d32d2e7](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud merged pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud merged pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#discussion_r462552385
##########
File path: superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
##########
@@ -260,6 +263,14 @@ class Tabs extends React.PureComponent {
onDeleteTab={this.handleDeleteTab}
/>
}
+ onExiting={() => {
+ // Exiting previous tab, animating start
+ this.props.setIsAnimating(true, tabId);
+ }}
+ onEntering={() => {
+ // Entering current tab, DOM is visible and has dimension
+ this.props.setIsAnimating(false, tabId);
+ }}
Review comment:
maybe they could be `onMount` and `onUnmount`? Or `afterMount` and `beforeUnmount`?
##########
File path: superset-frontend/src/dashboard/actions/dashboardState.js
##########
@@ -321,6 +321,14 @@ export function setDirectPathToChild(path) {
return { type: SET_DIRECT_PATH, path };
}
+export const SET_IS_TAB_MOUNTED = 'SET_IS_TAB_MOUNTED';
+/**
+ * Set if tab switch animation is in progress
+ */
+export function setIsTabMounted(isAnimating) {
Review comment:
maybe rename this variable to `isTabMounted` to match the function name?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-669319316
@ktmud The most recent changes look great, I'm not seeing the chart reload/repaint on tab switch anymore. I'll do more testing today and let you know if I notice anything.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666759093
I can fix the issue by modifying this line to set `realHeight` to `maxHeight` instead of `Math.min(...)`:
https://github.com/apache-superset/superset-ui/blob/master/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx#L170
Though not sure of other consequences for doing this. I found one issue when there are scrollbars. Perhaps the dimensions could be calculated instead of read from sticky state?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666770835
Yeah I refreshed a bunch of times and definitely have the latest code because I'm setting a breakpoint. So just to be clear, the steps are:
1. open dashboard for the first time
2. set the filter on the first tab to limit results to 1 row before ever switching to another tab
3. switch to the tab with the table.
4. switch back to the filter tab, clear the filter so more rows are returned
5. switch to the tab with the table
If you use docker-compose and use the BART lines dataset like I did in the video, I can reproduce it 100% of the time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666851845
That makes sense. I think this is OK though, other charts seem to behave this way
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666966298
Thank you, @bryanck ! I was able to reproduce with the exported dashboard. Will try to find a fix soon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-667205034
This change looks like it resolves the table layout issues related to tab switching. The only regression I've noticed is that there is more chart repainting going on during tab switches, previously a chart would rerender only if the data changed AFAIK. But otherwise this looks great.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-667007170
FYI I'm getting ready to cut `0.37.0rc4`. While this would be great to get in, this probably needs to wait for `0.37.1` to make sure we don't introduce any regressions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-668182336
@ktmud All dashboard component has prop `isComponentVisible`, which is updated when user switch tab:
https://github.com/apache/incubator-superset/blob/5b07c8d22901b7b173bd90f185904ea5c28bdad6/superset-frontend/src/dashboard/containers/DashboardComponent.jsx#L56
Chart will trigger new query (and render if necessary) according to this prop:
https://github.com/apache/incubator-superset/blob/5fa46804473ad1b3909aaef4bc1d4e729c048882/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx#L116
Could you check if table viz correctly update its props when it should (switch tab after apply filter)?
`slightly more chart repainting` is a very dangerous sign, because some viz type's render took pretty long, and all chart's render is blocking, which means it might cause browser freeze.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666847491
`onEntered` seems to fix it, `useLayoutEffect` gets called when the table is visible and the height is adjusted.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666635565
After applying this PR, I'm still seeing an issue with the max height being determined by the first render of the table (the first tab switch). Here's a video:
https://drive.google.com/file/d/1UvDdq-jV23VCwaORQauqyetNHQbmmtgp/view?usp=sharing
I'm also seeing timing issues where the height doesn't get set if you switch the tab fast enough, but that's harder to reproduce.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666850665
I'll change it to `onEntered` then. It's probably a browser/os thing. Didn't want to use `onEntered` initially because I wanted the layout to compute as soon as possible so the UI will seem less "jumpy".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666813607
It seems like the useLayoutEffect() gets called when the table is visible with the first tab switch, but after that, it gets called when hidden, thus the dimension setting is skipped bc theadHeight is 0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-668182336
@ktmud All dashboard component has prop `isComponentVisible`, which is updated when user switch tab, and tab(s) might be nested inside another tab:
https://github.com/apache/incubator-superset/blob/5b07c8d22901b7b173bd90f185904ea5c28bdad6/superset-frontend/src/dashboard/containers/DashboardComponent.jsx#L56
Chart will trigger new query (and render if necessary) according to this prop:
https://github.com/apache/incubator-superset/blob/5fa46804473ad1b3909aaef4bc1d4e729c048882/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx#L116
Could you check if table viz correctly update its props when it should (switch tab after apply filter)?
`slightly more chart repainting` is a very dangerous signal, because some viz type's render took pretty long, and all chart's render is blocking, which means it might cause browser freeze.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666962515
I tried this on a 2nd Mac I have, and I'm able to reproduce this issue on that machine as well, on both Chrome and Safari. I also tried both your fork and patching 0.37, same issue. Here's a link to the dashboard I'm using to test with, it uses the BART lines datasource available when you start the docker image - https://drive.google.com/file/d/1WMh3NjaNqrdVnLeno-DrrME0gnch4WQ-/view?usp=sharing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-667205034
This change looks like it resolves the table layout issues related to tab switching. The only regression I've noticed is that there is slightly more chart repainting going on during tab switches, previously a chart would rerender only if the data changed AFAIK. But otherwise this looks great.
I believe this should also fix a bug in previous versions where the table headers would become misaligned after a tab switch, e.g. https://github.com/apache/incubator-superset/issues/7524
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-668409487
why do we still need `SET_MOUNTED_TAB` action and related props? i assume `isComponentVisible` already did the job you need. What is root cause of this issue ( apply filter + switch tab didn't trigger table viz to update)? We don't have this issue before table viz refacotory, right? So i would prefer to fix from table viz instead of touch more files.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-667205034
This change looks like it resolves the table layout issues related to tab switching. The only regression I've noticed is that there is slightly more chart repainting going on during tab switches, previously a chart would rerender only if the data changed AFAIK. But otherwise this looks great.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666770835
Yeah I refreshed a bunch of times and definitely have the latest code because I'm setting a breakpoint. So just to be clear, the steps are:
1. open dashboard for the first time
2. set the filter on the first tab to limit results to 1 row before ever switching to another tab
3. switch to the tab with the table so it renders for the very first time with 1 row
4. switch back to the filter tab, clear the filter so more rows are returned
5. switch to the tab with the table
If you use docker-compose and use the BART lines dataset like I did in the video, I can reproduce it 100% of the time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666768650
That is needed to keep horizontal scroll bar close to the last row when the wrapper container is taller than the table. It is especially useful in Explore page when the chart almost takes the full height of the viewport.
I was not able to reproduce the issue with the latest fix. Did you refresh the page after checking out the latest commit?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666914358
I have another potential solution where you can keep the `onEntering`. It looks like the table chart is the only one that needs to be visible to calculate dimensions so probably the only one that needs `onEntered`. If you switch back to `onEntering` and make a small change in `useSticky.tsx` in the table plugin, and run the code in the `useLayoutEffect` asynchronously (e.g. via `setTimeout`), that allows the table to become visible before the code calculating the height is executed. I'm not sure if this is 100% reliable but it seems to be, and makes the tab switches much smoother again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666759093
I can fix the issue by modifying this line to set `realHeight` to `maxHeight` instead of `Math.min(...)`:
https://github.com/apache-superset/superset-ui/blob/master/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx#L170
Though not sure of other consequences for doing this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-668182336
@ktmud All dashboard component has prop `isComponentVisible`, which is updated when user switch tab, and tab(s) might be nested inside another tab:
https://github.com/apache/incubator-superset/blob/5b07c8d22901b7b173bd90f185904ea5c28bdad6/superset-frontend/src/dashboard/containers/DashboardComponent.jsx#L56
Chart will trigger new query (and render if necessary) according to this prop:
https://github.com/apache/incubator-superset/blob/5fa46804473ad1b3909aaef4bc1d4e729c048882/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx#L116
Could you check if table viz correctly update its props when it should (switch tab after apply filter)?
`slightly more chart repainting` is a very dangerous sign, because some viz type's render took pretty long, and all chart's render is blocking, which means it might cause browser freeze.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#discussion_r467510610
##########
File path: superset-frontend/src/dashboard/components/DashboardBuilder.jsx
##########
@@ -247,6 +248,11 @@ class DashboardBuilder extends React.Component {
<TabPane
key={index === 0 ? DASHBOARD_GRID_ID : id}
eventKey={index}
+ mountOnEnter
Review comment:
can we add an explicit `unmountOnExit={false}`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666930081
I'd prefer not to use react hooks in `setTimeout` or `setTiemout` in hooks since every time I used it, it turned out to be more problematic than the problem it tried to solve---and there was almost always a more robust solution.
Could you confirm which browsers and versions you tested are still seeing the problem with `onExiting` as the solution? Because I have tested Chrome, Safari and Firefox on Mac, and all of them cannot reproduce the bug.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#discussion_r462547648
##########
File path: superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
##########
@@ -260,6 +263,14 @@ class Tabs extends React.PureComponent {
onDeleteTab={this.handleDeleteTab}
/>
}
+ onExiting={() => {
+ // Exiting previous tab, animating start
+ this.props.setIsAnimating(true, tabId);
+ }}
+ onEntering={() => {
+ // Entering current tab, DOM is visible and has dimension
+ this.props.setIsAnimating(false, tabId);
+ }}
Review comment:
Use `onEntering` instead of `onEntered` because the contents for the new tab are already mounted to DOM upon entering and have values for dimension properties.
##########
File path: superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
##########
@@ -260,6 +263,14 @@ class Tabs extends React.PureComponent {
onDeleteTab={this.handleDeleteTab}
/>
}
+ onExiting={() => {
+ // Exiting previous tab, animating start
+ this.props.setIsAnimating(true, tabId);
+ }}
+ onEntering={() => {
+ // Entering current tab, DOM is visible and has dimension
+ this.props.setIsAnimating(false, tabId);
+ }}
Review comment:
This may be potentially a source of confusion because the animation didn't completely stop when `isAnimating = true`, but I'm struggling to find more appropriate + succinct variable names.
##########
File path: superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
##########
@@ -260,6 +263,14 @@ class Tabs extends React.PureComponent {
onDeleteTab={this.handleDeleteTab}
/>
}
+ onExiting={() => {
+ // Exiting previous tab, animating start
+ this.props.setIsAnimating(true, tabId);
+ }}
+ onEntering={() => {
+ // Entering current tab, DOM is visible and has dimension
+ this.props.setIsAnimating(false, tabId);
+ }}
Review comment:
Renamed to `isParentMounted` for charts and `isTabMounted` for dashboards.
##########
File path: superset-frontend/src/dashboard/actions/dashboardState.js
##########
@@ -321,6 +321,14 @@ export function setDirectPathToChild(path) {
return { type: SET_DIRECT_PATH, path };
}
+export const SET_IS_TAB_MOUNTED = 'SET_IS_TAB_MOUNTED';
+/**
+ * Set if tab switch animation is in progress
+ */
+export function setIsTabMounted(isAnimating) {
Review comment:
Oops, missed it while search and replace. Thanks for catching this!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-665895748
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-667205034
This change looks like it resolves the table layout issues related to tab switching. The only regression I've noticed is that there is slightly more chart repainting going on during tab switches, previously a chart would rerender only if the data changed AFAIK. But otherwise this looks great.
This may also fix a bug in previous versions where the table headers would become misaligned after a tab switch, e.g. https://github.com/apache/incubator-superset/issues/7524
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-665895748
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=h1) Report
> Merging [#10475](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7a329c25e9867636fd8a79c3f19b45186bd0f95a&el=desc) will **decrease** coverage by `0.41%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10475 +/- ##
==========================================
- Coverage 70.80% 70.39% -0.42%
==========================================
Files 604 196 -408
Lines 32426 19087 -13339
Branches 3414 0 -3414
==========================================
- Hits 22960 13436 -9524
+ Misses 9354 5651 -3703
+ Partials 112 0 -112
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `?` | |
| #python | `70.39% <ø> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/chart/chartReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L2NoYXJ0UmVkdWNlci5qcw==) | | |
| [superset-frontend/src/welcome/DashboardTable.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3dlbGNvbWUvRGFzaGJvYXJkVGFibGUudHN4) | | |
| [...et-frontend/src/dashboard/components/CodeModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0NvZGVNb2RhbC5qc3g=) | | |
| [...rc/dashboard/components/dnd/dragDroppableConfig.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2RuZC9kcmFnRHJvcHBhYmxlQ29uZmlnLmpz) | | |
| [...essageToasts/utils/getToastsFromPyFlashMessages.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL21lc3NhZ2VUb2FzdHMvdXRpbHMvZ2V0VG9hc3RzRnJvbVB5Rmxhc2hNZXNzYWdlcy5qcw==) | | |
| [...shboard/components/filterscope/FilterFieldItem.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2ZpbHRlcnNjb3BlL0ZpbHRlckZpZWxkSXRlbS5qc3g=) | | |
| [...tend/src/dashboard/util/getFilterScopeNodesTree.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlTm9kZXNUcmVlLmpz) | | |
| [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | | |
| [superset-frontend/src/CRUD/utils.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL0NSVUQvdXRpbHMuanM=) | | |
| [...d/src/dashboard/util/updateComponentParentsList.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL3VwZGF0ZUNvbXBvbmVudFBhcmVudHNMaXN0Lmpz) | | |
| ... and [392 more](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=footer). Last update [7a329c2...cd665c4](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-668362131
@graceguo-supercat Thanks for pointing me to the `gridComponents/Chart.jsx` file. Totally forgot we have this file, too.
Just [optimized](https://github.com/apache/incubator-superset/pull/10475/commits/ab93026c90b9abd1e781ef9a86eef0618ed2d195) my solution a little bit more and had more testing on re-renders (with console logging on `shouldComponentUpdate`). Didn't notice any noticeable performance regression.
@bryanck could you test again and lmk if there's any other issue coming up?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666770835
Yeah I refreshed a bunch of times and definitely have the latest code because I'm setting a breakpoint. So just to be clear, the steps are:
1. open dashboard for the first time
2. set the filter on the first tab to limit results to 1 row before ever switching to another tab
3. switch to the tab with the table so it renders for the very first time with 1 row
4. switch back to the filter tab, clear the filter so more rows are returned
5. switch to the tab with the table
If you use docker-compose and use the BART lines dataset like I did in the video, I can reproduce it 100% of the time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck edited a comment on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck edited a comment on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666851845
That makes sense. I think this is OK though, other charts seem render after being made visible most of the time anyway
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666933775
I've tested the same 3 browsers also on Mac (Catalina), all latest versions, no plugins, cleared the cache, verified the latest code is running, and it happens on all three 100% of the time, both with the simple example I screen captured, plus the main production dashboard we have. It could be a timing thing, perhaps on my machine the data is available very fast and the `useLayoutEffect`gets called early? I can create another screen capture if you want to see anything that would help
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bryanck commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
bryanck commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666862278
It is a little jumpy now, but I guess lesser of 2 evils.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666731254
@bryanck Thanks for testing! I think you just caught where the bug was. Just updated and it should work as expected now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-commenter commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-665895748
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=h1) Report
> Merging [#10475](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/259a344fd19c3df0d41adae6cb17e498272e33cc&el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `7.14%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10475 +/- ##
==========================================
- Coverage 65.97% 65.96% -0.01%
==========================================
Files 604 604
Lines 32384 32387 +3
Branches 3281 3283 +2
==========================================
- Hits 21365 21364 -1
- Misses 10831 10835 +4
Partials 188 188
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `59.67% <7.14%> (-0.05%)` | :arrow_down: |
| #python | `70.36% <ø> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `9.09% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `38.66% <0.00%> (-0.53%)` | :arrow_down: |
| [...d/src/dashboard/components/gridComponents/Tabs.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL1RhYnMuanN4) | `67.53% <0.00%> (-3.71%)` | :arrow_down: |
| [...nd/src/dashboard/containers/DashboardComponent.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZENvbXBvbmVudC5qc3g=) | `92.00% <ø> (ø)` | |
| [...-frontend/src/dashboard/reducers/dashboardState.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9yZWR1Y2Vycy9kYXNoYm9hcmRTdGF0ZS5qcw==) | `73.68% <0.00%> (-2.68%)` | :arrow_down: |
| [...t-frontend/src/dashboard/actions/dashboardState.js](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2Rhc2hib2FyZFN0YXRlLmpz) | `30.71% <33.33%> (+0.05%)` | :arrow_up: |
| [superset/errors.py](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXJyb3JzLnB5) | `93.54% <0.00%> (-0.21%)` | :arrow_down: |
| [superset/sql\_parse.py](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `99.30% <0.00%> (-0.01%)` | :arrow_down: |
| [superset/viz\_sip38.py](https://codecov.io/gh/apache/incubator-superset/pull/10475/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6X3NpcDM4LnB5) | `0.00% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=footer). Last update [259a344...f076595](https://codecov.io/gh/apache/incubator-superset/pull/10475?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-666996671
OK, the problem is with top-level tabs. It's a little tricky to fix, but I think I got it right this time.
Please checkout the latest code and lmk if you are still seeing the issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10475: fix(dashboard): add animation state to fix tab switch re-renders
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10475:
URL: https://github.com/apache/incubator-superset/pull/10475#issuecomment-668469454
Here's a bit more explanation: https://github.com/apache-superset/superset-ui/pull/703
Basically when `isComponentVisible` is updated and the new active tab rendered, it might still not have been mounted to the DOM, because of the tab switch animation.
The new table chart (and previously the world map) relies on rendered elements' `clientHeight` and `clientWidth` to compute the layout, which would not be available if the tab is not fully mounted. The old table chart didn't need this because it recomputes layout completely after React's rendering lifecycle. You'd notice it being "jumpier" than the current table chart.
This fix is more robust as it fixes the bug for any future visualizations that need element dimensions on the fly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org