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/03/03 23:14:12 UTC

[GitHub] [incubator-superset] ktmud opened a new pull request #9234: Perf: improve loading speed for legacy table chart

ktmud opened a new pull request #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234
 
 
   ### CATEGORY
   
   - [x] Enhancement (new features, refinement)
   
   ### SUMMARY
   
   One of the main pain points with the table chart is that it loads too slow for large datasets. This PR upgrades it to an updated version with improved performance. See [superset-ui-plugins PR#385](https://github.com/apache-superset/superset-ui-plugins/pull/385) for details.
   
   In addition to upgrade the table chart plugin, this PR also refactors some styling code related to it. 
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   Tested with a ~10,000 rows, 7 columns table in Superset, pagination enabled:
   
   ### Before 
   
   ![legacy-table--old](https://user-images.githubusercontent.com/335541/75824956-4b98d080-5d59-11ea-9f86-4a17d0770e6c.gif)
   
   Takes about 6 secs to load.
   
   ![Snip20200303_3](https://user-images.githubusercontent.com/335541/75825254-d8dc2500-5d59-11ea-8da1-8091c8afcc8b.png)
   
   
   ### After
   
   ![legacy-table--new](https://user-images.githubusercontent.com/335541/75824966-4dfb2a80-5d59-11ea-83a4-0a3468f9f7c4.gif)
   
   Takes only 3 secs to load!
   
   ![Snip20200303_4](https://user-images.githubusercontent.com/335541/75825271-dda0d900-5d59-11ea-98a9-e04fc2445aac.png)
   
   
   ### TEST PLAN
   
   - Query a large dataset, increase row limit to a very high number
   - Feel the difference in reactiveness and loading speed of the table chart
   
   ### ADDITIONAL INFORMATION
   
   - [x] Has associated issue:  this does not fix #8273 
   - [x] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [x] Removes existing feature or API
   
   ### REVIEWERS
   
   @kristw @graceguo-supercat @etr2460 @rusackas 

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 issue #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#issuecomment-594850779
 
 
   this change will automatic/force to show pagination on large table? Or it's up to user to show pagination or not?
   The perf improvement from 6 seconds to 3 seconds, is because the table only show top 50 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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r387357812
 
 

 ##########
 File path: superset-frontend/src/setup/setupApp.ts
 ##########
 @@ -67,7 +67,7 @@ export default function setupApp() {
     ) {
       ev.preventDefault();
       SupersetClient.get({
-        endpoint: ev.currentTarget.href,
+        url: ev.currentTarget.href,
 
 Review comment:
   This is also in #9215.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388672535
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   could use `const` and ternary 
   
   ```ts
   const chartClassName = vizType === 'table' ? `superset-chart-${chartClassName}` : snakeCase(vizType);
   ```

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388683345
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   Oh, I think another reason why I didn't use ternary is that the new class is based on an existing variable. It just feels more natural to use if statements to override it than either creating two variables or call `snakeCase(vizType)` twice.
   
   ```js
   let chartClassName = snakeCase(vizType);
   if (vizType === 'table') {
     chartClassName = `superset-chart-${chartClassName}`;
   }
   ```
   
   vs
   
   ```js
   let chartClassName = snakeCase(vizType);
   chartClassName = vizType === 'table' ? `superset-chart-${chartClassName}` : chartClassName;
   ```
   
   Still thinks the if block is more readable... 😬 
   
   vs 
   
   ```js
   const chartClassName = vizType === 'table' ? `superset-chart-${snakeCase(vizType)}` : snakeCase(vizType);
   ```

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388025323
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +229,18 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    if (isMigratedViz(vizType)) {
+      chartClassName = `superset-chart-${chartClassName}`;
+    }
+
     return (
       <>
         {this.renderTooltip()}
         <SuperChart
           disableErrorBoundary
           id={`chart-id-${chartId}`}
-          className={`${snakeCase(vizType)}`}
+          className={chartClassName}
 
 Review comment:
   ooo, nice catch!

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388681940
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   <strike>Makes sense, will update.</strike>

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r387936062
 
 

 ##########
 File path: superset/translations/zh/LC_MESSAGES/messages.json
 ##########
 @@ -3330,7 +3330,11 @@
       ],
       "Saved Queries": [
         "已保存查询"
 
 Review comment:
   we don't commit localization json file.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r389135501
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,21 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    chartClassName =
 
 Review comment:
   > What do you think of
   > 
   > ```ts
   > const snakeCaseVizType = snakeCase(vizType);
   > const chartClassName = vizType === 'table' ? `superset-chart-${snakeCaseVizType}` : snakeCaseVizType;
   > ```
   
   I like this one.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 issue #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#issuecomment-594850779
 
 
   this change will automatic/force to show pagination on large table? Or it's up to user to select show pagination?
   The perf improvement from 6 seconds to 3 seconds, is because the table only show top 50 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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 issue #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on issue #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#issuecomment-594856101
 
 
   > this change will automatic/force to show pagination on large table? Or it's up to user to select show pagination?
   > The perf improvement from 6 seconds to 3 seconds, is because the table only show top 50 rows?
   
   No, it will not. This PR only refactors the model and disabled a couple of already-broken features. The perf gain mainly comes from other optimizations.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r389133180
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,21 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    chartClassName =
 
 Review comment:
   What do you think of
   
   ```ts
   const snakeCaseVizType = snakeCase(vizType);
   const chartClassName = vizType === 'table' ? `superset-chart-${snakeCaseVizType}` : snakeCaseVizType;
   ```

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r389134591
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,21 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    chartClassName =
 
 Review comment:
   I generally avoid reassigning variables. `NVD3Vis.js` is an example of many reassignments that made it very difficult to refactor because order of operations matter a lot.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388684975
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   CI has passed. Can we merge as is?

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-io commented on issue #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#issuecomment-594986180
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9234?src=pr&el=h1) Report
   > Merging [#9234](https://codecov.io/gh/apache/incubator-superset/pull/9234?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/969bc87431264bb71462ca834b934fd295ed0650?src=pr&el=desc) will **increase** coverage by `<.01%`.
   > The diff coverage is `66.66%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9234/graphs/tree.svg?width=650&token=KsB0fHcx6l&height=150&src=pr)](https://codecov.io/gh/apache/incubator-superset/pull/9234?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9234      +/-   ##
   ==========================================
   + Coverage   58.93%   58.93%   +<.01%     
   ==========================================
     Files         373      373              
     Lines       12014    12017       +3     
     Branches     2945     2946       +1     
   ==========================================
   + Hits         7080     7082       +2     
   - Misses       4755     4756       +1     
     Partials      179      179
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9234?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9234/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `40.5% <66.66%> (+1.03%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9234?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/9234?src=pr&el=footer). Last update [969bc87...75234d1](https://codecov.io/gh/apache/incubator-superset/pull/9234?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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r389133180
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,21 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    chartClassName =
 
 Review comment:
   ```ts
   const snakeCaseVizType = snakeCase(vizType);
   const chartClassName = vizType === 'table' ? `superset-chart-${snakeCaseVizType}` : snakeCaseVizType;
   ```

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] ktmud edited a comment on issue #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on issue #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#issuecomment-594856101
 
 
   > this change will automatic/force to show pagination on large table? Or it's up to user to select show pagination?
   > The perf improvement from 6 seconds to 3 seconds, is because the table only show top 50 rows?
   
   No, it will not. This PR only refactors the chart module and disabled a couple of already-broken features. The perf gain mainly comes from other optimizations.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388025218
 
 

 ##########
 File path: superset-frontend/src/setup/setupApp.ts
 ##########
 @@ -67,7 +67,7 @@ export default function setupApp() {
     ) {
       ev.preventDefault();
       SupersetClient.get({
-        endpoint: ev.currentTarget.href,
+        url: ev.currentTarget.href,
 
 Review comment:
   let's revert this change from this pr then if it's in a different one already

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r387357494
 
 

 ##########
 File path: superset-frontend/src/dashboard/stylesheets/dashboard.less
 ##########
 @@ -60,6 +60,7 @@ body {
   position: relative;
   font-size: @font-size-l;
   font-weight: @font-weight-bold;
+  margin-bottom: 3px;
 
 Review comment:
   ![image](https://user-images.githubusercontent.com/335541/75829755-e4cce480-5d63-11ea-9cd4-f559b616a41f.png)
   
   This adds a little space between dashboard chart container header and visualizations.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] kristw edited a comment on issue #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
kristw edited a comment on issue #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#issuecomment-595555115
 
 
   @ktmud Please resolve conflict

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388674622
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   Was planning to allow more viz types in the future.
   
   ```js
    if (vizType === 'table' || vizType === 'another-one' ) {
   ```
   
   Ternary makes this code look one-off but we actually have more planned.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388681940
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   Makes sense, will update.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 issue #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#issuecomment-594850779
 
 
   this change will automatic/force to show pagination on large table? Or it's up to user to select show pagination?
   The perf improvement from 6 seconds to 3 seconds, will only help table with pagination, or all table charts?

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388674622
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   Was planning to allow more viz types in the future.
   
   ```
    if (vizType === 'table' || vizType === 'another-one' ) {
   ```
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388681940
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   Makes sense, will update.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388683345
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   Oh, I think another reason why I didn't use ternary is that the new class is based on an existing variable. It just feels more natural to override it via if statement than either creating two variables or call `snakeCase(vizType)` twice.
   
   ```js
   let chartClassName = snakeCase(vizType);
   if (vizType === 'table') {
     chartClassName = `superset-chart-${chartClassName}`;
   }
   ```
   
   vs
   
   ```js
   let chartClassName = snakeCase(vizType);
   chartClassName = vizType === 'table' ? `superset-chart-${chartClassName}` : chartClassName;
   ```
   
   vs 
   
   ```js
   const chartClassName = vizType === 'table' ? `superset-chart-${snakeCase(vizType)}` : snakeCase(vizType);
   ```
   
   Still thinks the if block is more readable... 😬 
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388681940
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   Makes sense, <strike>will update.</strike>

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r387936062
 
 

 ##########
 File path: superset/translations/zh/LC_MESSAGES/messages.json
 ##########
 @@ -3330,7 +3330,11 @@
       ],
       "Saved Queries": [
         "已保存查询"
 
 Review comment:
   we don't commit localization json file.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r387355553
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +229,18 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    if (isMigratedViz(vizType)) {
+      chartClassName = `superset-chart-${chartClassName}`;
+    }
+
     return (
       <>
         {this.renderTooltip()}
         <SuperChart
           disableErrorBoundary
           id={`chart-id-${chartId}`}
-          className={`${snakeCase(vizType)}`}
+          className={chartClassName}
 
 Review comment:
   This will change (fix) the looks of the table chart.
   
   Previously the whole table has a gray background:
   ![Snip20200303_4](https://user-images.githubusercontent.com/335541/75828912-c5cd5300-5d61-11ea-90de-1ee451c647c9.png)
   
   which seems to be an unintentional because it comes from a CSS rule `.table .table` for [nested Bootstrap table](https://github.com/twbs/bootstrap/blob/68b0d231a13201eb14acd3dc84e51543d16e5f7e/less/tables.less#L84-L87). 
   
   Removing the background does also give us the strips:
   
   ![image](https://user-images.githubusercontent.com/335541/75829481-26a95b00-5d63-11ea-8191-1f8e47bdeb1b.png)
   
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r389136103
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,21 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    chartClassName =
 
 Review comment:
   > I generally avoid reassigning variables. `NVD3Vis.js` is an example of many reassignments that made it very difficult to refactor because order of operations matter a lot.
   
   I think it makes sense to use a combination of both. `const` should be default, but `let` can be acceptable in small scopes.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388680803
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   you could still use ternary for that when adding more `vizType` or use set if the list grows longer.
   ```ts
   const visSet = new Set([...]);
   const chartClassName = visSet.has(vizType) ? ... : ... ;
   ```

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] kristw merged pull request #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
kristw merged pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234
 
 
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388683345
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   Oh, I think another reason why I didn't use ternary is that the new class is based on an existing variable. It just feels more natural to use if statements to override it than either creating two variables or call `snakeCase(vizType)` twice.
   
   ```js
   let chartClassName = snakeCase(vizType);
   if (vizType === 'table') {
     chartClassName = `superset-chart-${chartClassName}`;
   }
   ```
   
   vs
   
   ```js
   let chartClassName = snakeCase(vizType);
   chartClassName = vizType === 'table' ? `superset-chart-${chartClassName}` : chartClassName;
   ```
   
   vs 
   
   ```js
   const chartClassName = vizType === 'table' ? `superset-chart-${snakeCase(vizType)}` : snakeCase(vizType);
   ```
   
   Still thinks the if block is more readable... 😬 
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: Perf: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9234: Perf: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388025145
 
 

 ##########
 File path: superset/translations/zh/LC_MESSAGES/messages.json
 ##########
 @@ -3330,7 +3330,11 @@
       ],
       "Saved Queries": [
         "已保存查询"
-      ]
+      ],
+      "Prev": ["上一页"],
+      "Next": ["下一页"],
+      "First": ["首页"],
+      "Last": ["尾页"]
 
 Review comment:
   could we move this out into a seperate PR? In general a PR should be a standalone chunk of work

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
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 #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#discussion_r388684975
 
 

 ##########
 File path: superset-frontend/src/chart/ChartRenderer.jsx
 ##########
 @@ -222,13 +222,22 @@ class ChartRenderer extends React.Component {
       queryResponse,
     } = this.props;
 
+    let chartClassName = snakeCase(vizType);
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    if (vizType === 'table') {
 
 Review comment:
   CI has passed. Can we merge as is?

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-io edited a comment on issue #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#issuecomment-594986180
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9234?src=pr&el=h1) Report
   > Merging [#9234](https://codecov.io/gh/apache/incubator-superset/pull/9234?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/000a038af193f2770dfe23a053bd1ff5e9e72bd4?src=pr&el=desc) will **increase** coverage by `0.01%`.
   > The diff coverage is `100%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9234/graphs/tree.svg?width=650&token=KsB0fHcx6l&height=150&src=pr)](https://codecov.io/gh/apache/incubator-superset/pull/9234?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9234      +/-   ##
   ==========================================
   + Coverage   58.93%   58.94%   +0.01%     
   ==========================================
     Files         373      373              
     Lines       12014    12017       +3     
     Branches     2945     2946       +1     
   ==========================================
   + Hits         7080     7083       +3     
     Misses       4755     4755              
     Partials      179      179
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9234?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9234/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `41.77% <100%> (+2.29%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9234?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/9234?src=pr&el=footer). Last update [000a038...1500051](https://codecov.io/gh/apache/incubator-superset/pull/9234?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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] kristw commented on issue #9234: feat: improve loading speed for legacy table chart

Posted by GitBox <gi...@apache.org>.
kristw commented on issue #9234: feat: improve loading speed for legacy table chart
URL: https://github.com/apache/incubator-superset/pull/9234#issuecomment-595555115
 
 
   @ktmud Please rebase

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org