You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2024/12/20 13:09:24 UTC
[PR] refactor: Removes 5.0 approved legacy charts [superset]
michael-s-molina opened a new pull request, #31582:
URL: https://github.com/apache/superset/pull/31582
### SUMMARY
As part of the 5.0 breaking window, this PR executes the following proposals:
- [23 - Force legacy Area chart migration and delete NVD3 plugin](https://github.com/orgs/apache/projects/345/views/1#)
- [42 - Migrate Line Chart (legacy) to ECharts Line Chart and remove code (was previously deprecated)](https://github.com/orgs/apache/projects/345/views/1#)
- [54 - Migrate and remove the legacy Heatmap chart](https://github.com/orgs/apache/projects/345/views/1#)
- [80 - Migrate and remove the NVD3 Bar Chart in favor of ECharts](https://github.com/orgs/apache/projects/345/views/1#)
- [84 - Migrate and remove the legacy Sankey chart](https://github.com/orgs/apache/projects/345/views/1#)
- [85 - Remove the unused Sankey Loop chart](https://github.com/orgs/apache/projects/345/views/1#)
- [86 - Migrate and remove the legacy Histogram chart](https://github.com/orgs/apache/projects/345/views/1#)
- [88 - Remove the Event Flow chart](https://github.com/orgs/apache/projects/345/views/1#)
- [89 - Migrate and remove the legacy Nightingale chart](https://github.com/orgs/apache/projects/345/views/1#)
I decided to do it all in a single PR to have a single migration for the charts. There's also many code blocks that were removed that touches multiple visualization types at the same time and it would be more complex to remove it in parts. Another important factor was the ability to execute the tests once, given that we need to check if the example charts are loaded correctly. In summary, the benefits outweigh the review complexity.
TODO:
- Add the migration
- Remove rose plugin
### TESTING INSTRUCTIONS
- Check the examples are loaded correctly
- Check that the charts in your production environment are all migrated and loaded correctly
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2557542063
<img src="https://media1.giphy.com/media/wnmfA4N6mIziIGEKEs/giphy.gif"/>
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2578220259
OMG -10k LoC !!!!!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1907469384
##########
superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts:
##########
@@ -255,13 +243,16 @@ function saveExploreColorScheme(
cy.wait('@chartUpdate');
}
+// FIXME: Skipping some tests as ECharts are rendered using Canvas and we cannot inspect the elements
Review Comment:
It feels like we _could_ optimize this test, too, and use example dashboards to do fewer round trips between Dashboard and Chart Editor (though, maybe optimization is secondary, and round trip testing is a worthy effort too ¯\_(ツ)_/¯)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #31582:
URL: https://github.com/apache/superset/pull/31582
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1907473100
##########
superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts:
##########
@@ -24,9 +24,9 @@ export const WORLD_HEALTH_CHARTS = [
{ name: '% Rural', viz: 'world_map' },
{ name: 'Most Populated Countries', viz: 'table' },
{ name: "World's Population", viz: 'big_number' },
- { name: 'Growth Rate', viz: 'line' },
+ { name: 'Growth Rate', viz: 'echarts_timeseries_line' },
Review Comment:
Not a problem, but maybe we can rename the new ones at some point since they support categorical and are no longer timeseries charts, really.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2581139655
WOOHOO!!!!!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1908724560
##########
superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts:
##########
@@ -24,9 +24,9 @@ export const WORLD_HEALTH_CHARTS = [
{ name: '% Rural', viz: 'world_map' },
{ name: 'Most Populated Countries', viz: 'table' },
{ name: "World's Population", viz: 'big_number' },
- { name: 'Growth Rate', viz: 'line' },
+ { name: 'Growth Rate', viz: 'echarts_timeseries_line' },
Review Comment:
Yeah. We should definitely do that in the future.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1907514337
##########
superset-frontend/package-lock.json:
##########
@@ -61783,161 +61392,6 @@
}
}
},
- "@data-ui/histogram": {
- "version": "0.0.84",
- "requires": {
- "@data-ui/shared": "^0.0.84",
- "@data-ui/theme": "^0.0.84",
- "@vx/axis": "^0.0.179",
- "@vx/curve": "^0.0.165",
- "@vx/event": "^0.0.179",
- "@vx/glyph": "^0.0.179",
- "@vx/gradient": "^0.0.165",
- "@vx/group": "^0.0.170",
- "@vx/pattern": "^0.0.179",
- "@vx/responsive": "^0.0.192",
- "@vx/scale": "^0.0.179",
- "@vx/shape": "^0.0.179",
- "@vx/tooltip": "0.0.179",
Review Comment:
Woohoo! Farewell old junk, hello React upgrade potential!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1907512797
##########
superset-frontend/cypress-base/cypress/support/e2e.ts:
##########
@@ -36,9 +36,9 @@ const { getConfig, setConfig } = failOnConsoleError({
],
});
-// Set individual tests to allow certain console errors to NOT fail, e.g
-// cy.allowConsoleErrors(['foo', /^some bar-regex.*/]);
-// This will be reset between tests.
+// // Set individual tests to allow certain console errors to NOT fail, e.g
Review Comment:
Probably don't need to double comment, but whatevs ;)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2577840213
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2581013063
@betodealmeida @eschutho @mistercrunch Can you stamp this PR as code owners?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "ryepezb1 (via GitHub)" <gi...@apache.org>.
ryepezb1 commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2649791237
cuando lanzan superset 5 ??
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2650716728
@ryepezb1 You check the schedule for major releases in our [Release Process](https://github.com/apache/superset/wiki/Release-Process#schedule).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1907455592
##########
superset/migrations/versions/2025-01-08_09-34_d482d51c15ca_remove_legacy_plugins_5_0.py:
##########
@@ -0,0 +1,78 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""remove_legacy_plugins_5_0
+
+Revision ID: d482d51c15ca
+Revises: eb1c288c71c4
+Create Date: 2025-01-08 09:34:57.533332
+
+"""
+
+from alembic import op
+
+from superset import db
+from superset.migrations.shared.migrate_viz.processors import (
+ MigrateAreaChart,
+ MigrateBarChart,
+ MigrateDistBarChart,
+ MigrateHeatmapChart,
+ MigrateHistogramChart,
+ MigrateLineChart,
+ MigrateSankey,
+)
+
+# revision identifiers, used by Alembic.
+revision = "d482d51c15ca"
+down_revision = "eb1c288c71c4"
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+ try:
+ MigrateAreaChart.upgrade(session)
+ MigrateBarChart.upgrade(session)
+ MigrateDistBarChart.upgrade(session)
+ MigrateHeatmapChart.upgrade(session)
+ MigrateHistogramChart.upgrade(session)
+ MigrateLineChart.upgrade(session)
+ MigrateSankey.upgrade(session)
+ session.commit()
+ except Exception as e:
+ session.rollback()
+ raise Exception(f"Error upgrading legacy viz types: {e}") from e
Review Comment:
### Incorrect Error Message in Downgrade Function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error message in the downgrade function incorrectly states 'Error upgrading legacy viz types' when it should be 'Error downgrading legacy viz types'
###### Why this matters
This incorrect error message could mislead developers debugging issues during a downgrade operation, potentially causing confusion and increasing time to resolve issues.
###### Suggested change
Change the error message to: raise Exception(f"Error downgrading legacy viz types: {e}") from e
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:5b41490c-6c54-41b4-95a3-a41cfe6d458c -->
##########
superset/migrations/versions/2025-01-08_09-34_d482d51c15ca_remove_legacy_plugins_5_0.py:
##########
@@ -0,0 +1,78 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""remove_legacy_plugins_5_0
+
+Revision ID: d482d51c15ca
+Revises: eb1c288c71c4
+Create Date: 2025-01-08 09:34:57.533332
+
+"""
+
+from alembic import op
+
+from superset import db
+from superset.migrations.shared.migrate_viz.processors import (
+ MigrateAreaChart,
+ MigrateBarChart,
+ MigrateDistBarChart,
+ MigrateHeatmapChart,
+ MigrateHistogramChart,
+ MigrateLineChart,
+ MigrateSankey,
+)
+
+# revision identifiers, used by Alembic.
+revision = "d482d51c15ca"
+down_revision = "eb1c288c71c4"
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+ try:
+ MigrateAreaChart.upgrade(session)
+ MigrateBarChart.upgrade(session)
+ MigrateDistBarChart.upgrade(session)
+ MigrateHeatmapChart.upgrade(session)
+ MigrateHistogramChart.upgrade(session)
+ MigrateLineChart.upgrade(session)
+ MigrateSankey.upgrade(session)
+ session.commit()
+ except Exception as e:
+ session.rollback()
+ raise Exception(f"Error upgrading legacy viz types: {e}") from e
Review Comment:
### Non-specific error message in chart migration <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error message is too generic and doesn't indicate which specific chart migration failed.
###### Why this matters
When an error occurs, it will be difficult to identify which chart migration caused the issue, leading to slower debugging and resolution times.
###### Suggested change
Include the name of the failing chart migration in the error message:
try:
for migration in [MigrateAreaChart, MigrateBarChart, ...]:
migration.upgrade(session)
session.commit()
except Exception as e:
session.rollback()
raise Exception(f"Error upgrading {migration.__name__}: {e}") from e
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:b25bfc9b-b826-40c5-a200-1f314ef16955 -->
##########
superset/examples/birth_names.py:
##########
@@ -239,7 +239,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
Slice(
**slice_kwargs,
slice_name="Genders by State",
- viz_type="dist_bar",
+ viz_type="echarts_timeseries_bar",
Review Comment:
### Incorrect Chart Type for State Data <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The chart's visualization type 'echarts_timeseries_bar' is incorrect for displaying gender distribution by state, which is not a time series data.
###### Why this matters
Using a time series visualization for non-time series data will result in incorrect data representation and could mislead users looking at state-wise gender distribution.
###### Suggested change
Replace 'echarts_timeseries_bar' with 'echarts_bar' or 'dist_bar' which are more appropriate for displaying categorical state-wise comparisons.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:1fd30521-0d83-48bd-94db-9f507c13aad6 -->
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -132,7 +132,7 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> {
});
}
} catch (error) {
- this.props.actions.addDangerToast(
+ this.props.addDangerToast(
t('An error occurred while loading dashboard information.'),
);
Review Comment:
### Lost Error Context in Toast Message <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error catch block discards the error details when loading dashboard information.
###### Why this matters
Loss of error context makes debugging production issues more difficult since the actual cause of the failure is not captured.
###### Suggested change
Include error details in the toast message: `t('An error occurred while loading dashboard information: ${error.message}')`
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:27656e28-cdcf-4ecf-84f0-f94ec1b41f5b -->
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1907480268
##########
superset-frontend/cypress-base/cypress/e2e/explore/control.test.ts:
##########
@@ -157,107 +156,9 @@ describe('Test datatable', () => {
});
});
-describe('Time range filter', () => {
Review Comment:
Not sure yet if this one just got relocated, but it looks potentially important?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2578077083
/korbit-review
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2578080368
Reviewing... bear with me for a bit.
<img src="https://media1.giphy.com/media/nOW7pC0POeyOID23T3/giphy.gif"/>
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1907482729
##########
superset-frontend/cypress-base/cypress/e2e/explore/control.test.ts:
##########
@@ -157,107 +156,9 @@ describe('Test datatable', () => {
});
});
-describe('Time range filter', () => {
Review Comment:
This test was about the legacy time section that pre-dated generic x-axis.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2556974164
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1905541019
##########
superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts:
##########
@@ -255,13 +243,16 @@ function saveExploreColorScheme(
cy.wait('@chartUpdate');
}
+// FIXME: Skipping some tests as ECharts are rendered using Canvas and we cannot inspect the elements
Review Comment:
@geido The color scheme tests were using a legacy chart to check the applied colors. Given that ECharts are rendered using Canvas, we'll need a solution to be able to inspect the elements. I'm skipping these tests for now and adding a `FIXME` comment.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #31582:
URL: https://github.com/apache/superset/pull/31582#issuecomment-2577634364
Korbit doesn't automatically review large (500+ lines changed) pull requests such as this one. If you want me to review anyway, use `/korbit-review`.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] refactor: Removes 5.0 approved legacy charts [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #31582:
URL: https://github.com/apache/superset/pull/31582#discussion_r1907475559
##########
superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts:
##########
@@ -255,13 +243,16 @@ function saveExploreColorScheme(
cy.wait('@chartUpdate');
}
+// FIXME: Skipping some tests as ECharts are rendered using Canvas and we cannot inspect the elements
Review Comment:
Yeah, that's a good suggestion. One option I can think of to overcome the ECharts canvas problem and be able to identify color changes would be to use the Word Cloud plugin which is pure HTML. I believe the solution for this problem should be resolved in a follow-up PR. I already sent a message to @geido about 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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