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/23 22:37:52 UTC
[GitHub] [incubator-superset] rusackas opened a new pull request #9355: Cal
heatmap controls migration
rusackas opened a new pull request #9355: Cal heatmap controls migration
URL: https://github.com/apache/incubator-superset/pull/9355
### CATEGORY
Choose one
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [x] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Moving unique control(s) from `controls.jsx` to `/explore/controlPanels/CalHeatmap.js`
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@kristw
----------------------------------------------------------------
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] villebro commented on a change in pull request
#9355: Cal heatmap controls migration
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9355: Cal heatmap controls migration
URL: https://github.com/apache/incubator-superset/pull/9355#discussion_r396908889
##########
File path: superset-frontend/src/explore/controlPanels/CalHeatmap.js
##########
@@ -35,21 +78,94 @@ export default {
expanded: true,
controlSetRows: [
['linear_color_scheme'],
- ['cell_size', 'cell_padding'],
- ['cell_radius', 'steps'],
- ['y_axis_format', 'x_axis_time_format'],
+ [
+ {
+ name: 'cell_size',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ default: 10,
+ validators: [v.integer],
+ renderTrigger: true,
+ label: t('Cell Size'),
+ description: t('The size of the square cell, in pixels'),
+ },
+ },
+ {
+ name: 'cell_padding',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 2,
+ label: t('Cell Padding'),
+ description: t('The distance between cells, in pixels'),
+ },
+ },
+ ],
+ [
+ {
+ name: 'cell_radius',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 0,
+ label: t('Cell Radius'),
+ description: t('The pixel radius'),
+ },
+ },
+ {
+ name: 'steps',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 10,
+ label: t('Color Steps'),
+ description: t('The number color "steps"'),
+ },
+ },
+ ],
+ [
+ 'y_axis_format',
+ {
+ name: 'x_axis_time_format',
Review comment:
@runsackas I like the idea of doing a migration batch to clean up duplicate controls. Let's hold off on the changes for now and put them in a google sheet or similar so we can do one or two migrations when the control migration is otherwise done.
----------------------------------------------------------------
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] rusackas commented on a change in pull request
#9355: Cal heatmap controls migration
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9355: Cal heatmap controls migration
URL: https://github.com/apache/incubator-superset/pull/9355#discussion_r396799250
##########
File path: superset-frontend/src/explore/controlPanels/CalHeatmap.js
##########
@@ -35,21 +78,94 @@ export default {
expanded: true,
controlSetRows: [
['linear_color_scheme'],
- ['cell_size', 'cell_padding'],
- ['cell_radius', 'steps'],
- ['y_axis_format', 'x_axis_time_format'],
+ [
+ {
+ name: 'cell_size',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ default: 10,
+ validators: [v.integer],
+ renderTrigger: true,
+ label: t('Cell Size'),
+ description: t('The size of the square cell, in pixels'),
+ },
+ },
+ {
+ name: 'cell_padding',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 2,
+ label: t('Cell Padding'),
+ description: t('The distance between cells, in pixels'),
+ },
+ },
+ ],
+ [
+ {
+ name: 'cell_radius',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 0,
+ label: t('Cell Radius'),
+ description: t('The pixel radius'),
+ },
+ },
+ {
+ name: 'steps',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 10,
+ label: t('Color Steps'),
+ description: t('The number color "steps"'),
+ },
+ },
+ ],
+ [
+ 'y_axis_format',
+ {
+ name: 'x_axis_time_format',
Review comment:
@kristw it appears I can change this one to use `x_axis_format` with a couple config overrides. This would get rid of `x_axis_time_format` entirely, which appears to be a deprecated practice.
If that's a good idea, I can make the change here, and a quick PR in `superset-ui-plugins` to match.
However, I'm not sure if this leads to any painful migrations for existing charts/dashboards. Thoughts?
----------------------------------------------------------------
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] villebro commented on a change in pull request
#9355: Cal heatmap controls migration
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9355: Cal heatmap controls migration
URL: https://github.com/apache/incubator-superset/pull/9355#discussion_r396908889
##########
File path: superset-frontend/src/explore/controlPanels/CalHeatmap.js
##########
@@ -35,21 +78,94 @@ export default {
expanded: true,
controlSetRows: [
['linear_color_scheme'],
- ['cell_size', 'cell_padding'],
- ['cell_radius', 'steps'],
- ['y_axis_format', 'x_axis_time_format'],
+ [
+ {
+ name: 'cell_size',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ default: 10,
+ validators: [v.integer],
+ renderTrigger: true,
+ label: t('Cell Size'),
+ description: t('The size of the square cell, in pixels'),
+ },
+ },
+ {
+ name: 'cell_padding',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 2,
+ label: t('Cell Padding'),
+ description: t('The distance between cells, in pixels'),
+ },
+ },
+ ],
+ [
+ {
+ name: 'cell_radius',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 0,
+ label: t('Cell Radius'),
+ description: t('The pixel radius'),
+ },
+ },
+ {
+ name: 'steps',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 10,
+ label: t('Color Steps'),
+ description: t('The number color "steps"'),
+ },
+ },
+ ],
+ [
+ 'y_axis_format',
+ {
+ name: 'x_axis_time_format',
Review comment:
@rusackas I like the idea of doing a migration batch to clean up duplicate controls. Let's hold off on the changes for now and put them in a google sheet or similar so we can do one or two migrations when the control migration is otherwise done.
----------------------------------------------------------------
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] rusackas merged pull request #9355: Cal
heatmap controls migration
Posted by GitBox <gi...@apache.org>.
rusackas merged pull request #9355: Cal heatmap controls migration
URL: https://github.com/apache/incubator-superset/pull/9355
----------------------------------------------------------------
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] williaster commented on issue #9355: Cal
heatmap controls migration
Posted by GitBox <gi...@apache.org>.
williaster commented on issue #9355: Cal heatmap controls migration
URL: https://github.com/apache/incubator-superset/pull/9355#issuecomment-602976603
so excited for this! 👯
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
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
#9355: Cal heatmap controls migration
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9355: Cal heatmap controls migration
URL: https://github.com/apache/incubator-superset/pull/9355#discussion_r396846043
##########
File path: superset-frontend/src/explore/controlPanels/CalHeatmap.js
##########
@@ -35,21 +78,94 @@ export default {
expanded: true,
controlSetRows: [
['linear_color_scheme'],
- ['cell_size', 'cell_padding'],
- ['cell_radius', 'steps'],
- ['y_axis_format', 'x_axis_time_format'],
+ [
+ {
+ name: 'cell_size',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ default: 10,
+ validators: [v.integer],
+ renderTrigger: true,
+ label: t('Cell Size'),
+ description: t('The size of the square cell, in pixels'),
+ },
+ },
+ {
+ name: 'cell_padding',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 2,
+ label: t('Cell Padding'),
+ description: t('The distance between cells, in pixels'),
+ },
+ },
+ ],
+ [
+ {
+ name: 'cell_radius',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 0,
+ label: t('Cell Radius'),
+ description: t('The pixel radius'),
+ },
+ },
+ {
+ name: 'steps',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 10,
+ label: t('Color Steps'),
+ description: t('The number color "steps"'),
+ },
+ },
+ ],
+ [
+ 'y_axis_format',
+ {
+ name: 'x_axis_time_format',
Review comment:
Umm, as much as I wanna merge them, this will cause `db:migration` for existing charts/dashboards so perhaps let's hold on to it.
----------------------------------------------------------------
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] rusackas commented on a change in pull request
#9355: Cal heatmap controls migration
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9355: Cal heatmap controls migration
URL: https://github.com/apache/incubator-superset/pull/9355#discussion_r396906527
##########
File path: superset-frontend/src/explore/controlPanels/CalHeatmap.js
##########
@@ -35,21 +78,94 @@ export default {
expanded: true,
controlSetRows: [
['linear_color_scheme'],
- ['cell_size', 'cell_padding'],
- ['cell_radius', 'steps'],
- ['y_axis_format', 'x_axis_time_format'],
+ [
+ {
+ name: 'cell_size',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ default: 10,
+ validators: [v.integer],
+ renderTrigger: true,
+ label: t('Cell Size'),
+ description: t('The size of the square cell, in pixels'),
+ },
+ },
+ {
+ name: 'cell_padding',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 2,
+ label: t('Cell Padding'),
+ description: t('The distance between cells, in pixels'),
+ },
+ },
+ ],
+ [
+ {
+ name: 'cell_radius',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 0,
+ label: t('Cell Radius'),
+ description: t('The pixel radius'),
+ },
+ },
+ {
+ name: 'steps',
+ config: {
+ type: 'TextControl',
+ isInt: true,
+ validators: [v.integer],
+ renderTrigger: true,
+ default: 10,
+ label: t('Color Steps'),
+ description: t('The number color "steps"'),
+ },
+ },
+ ],
+ [
+ 'y_axis_format',
+ {
+ name: 'x_axis_time_format',
Review comment:
Ok... I'll try to keep track of that, as it may be worth addressing this and other similar migrations... maybe we can get a handful of them in a batch. Heads up to @villebro or @willbarrett that this may come up in discussion.
----------------------------------------------------------------
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