You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2023/12/05 03:43:59 UTC
[PR] feat: update time comparison choices (again) [superset]
john-bodley opened a new pull request, #26173:
URL: https://github.com/apache/superset/pull/26173
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
<!--- Describe the change below, including rationale and design decisions -->
Similar to https://github.com/apache/superset/pull/22458, this PR adds Y5Y time comparison choices to make it faster to compare with pre-pandemic times.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
CI.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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] feat: update time comparison choices (again) [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #26173:
URL: https://github.com/apache/superset/pull/26173#issuecomment-1843507573
Closing in favor of implementing an internal custom override. It seems like Y4Y, Y5Y, etc. comparisons are very much organization dependent.
--
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] feat: update time comparison choices (again) [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #26173:
URL: https://github.com/apache/superset/pull/26173#discussion_r1416061961
##########
superset-frontend/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx:
##########
@@ -322,6 +322,10 @@ const config: ControlPanelConfig = {
['2 years', t('2 years')],
['156 weeks', t('156 weeks')],
['3 years', t('3 years')],
+ ['208 weeks ago', t('208 weeks ago')],
Review Comment:
As Y4Y comparison was removed in https://github.com/apache/superset/pull/22150, the custom time shift option can vary in different scenarios. To ensure consistency, it is advisable to override the custom option by utilizing the chartControlRegistry on the vendor side.
--
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] feat: update time comparison choices (again) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #26173:
URL: https://github.com/apache/superset/pull/26173#discussion_r1416110792
##########
superset-frontend/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx:
##########
@@ -322,6 +322,10 @@ const config: ControlPanelConfig = {
['2 years', t('2 years')],
['156 weeks', t('156 weeks')],
['3 years', t('3 years')],
+ ['208 weeks ago', t('208 weeks ago')],
Review Comment:
Thanks @justinpark for the additional context. @john-bodley I wonder if we should make these configurable somehow.
--
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] feat: update time comparison choices (again) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #26173:
URL: https://github.com/apache/superset/pull/26173#discussion_r1415550342
##########
superset-frontend/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx:
##########
@@ -322,6 +322,10 @@ const config: ControlPanelConfig = {
['2 years', t('2 years')],
['156 weeks', t('156 weeks')],
['3 years', t('3 years')],
+ ['208 weeks ago', t('208 weeks ago')],
Review Comment:
@john-bodley Is there are reason to keep the time period in weeks when they match the year? Why not use weeks only when the number of weeks is not a multiple of 52?
--
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] feat: update time comparison choices (again) [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #26173:
URL: https://github.com/apache/superset/pull/26173#discussion_r1416061961
##########
superset-frontend/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx:
##########
@@ -322,6 +322,10 @@ const config: ControlPanelConfig = {
['2 years', t('2 years')],
['156 weeks', t('156 weeks')],
['3 years', t('3 years')],
+ ['208 weeks ago', t('208 weeks ago')],
Review Comment:
As @michael-s-molina mentioned, the custom time shift option can vary in different scenarios. To ensure consistency, it is advisable to override the custom option by utilizing the chartControlRegistry on the vendor side.
--
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] feat: update time comparison choices (again) [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley closed pull request #26173: feat: update time comparison choices (again)
URL: https://github.com/apache/superset/pull/26173
--
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] feat: update time comparison choices (again) [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #26173:
URL: https://github.com/apache/superset/pull/26173#discussion_r1414838473
##########
superset-frontend/packages/superset-ui-chart-controls/src/sections/advancedAnalytics.tsx:
##########
@@ -121,6 +121,10 @@ export const advancedAnalyticsControls: ControlPanelSectionConfig = {
['2 years ago', t('2 years ago')],
['156 weeks ago', t('156 weeks ago')],
['3 years ago', t('3 years ago')],
+ ['208 weeks ago', t('208 weeks ago')],
Review Comment:
It seems like the Y4Y comparison was removed in https://github.com/apache/superset/pull/22150, though it's not evident—per the PR description and/or comments—as to why, hence I re-added them.
##########
superset-frontend/packages/superset-ui-chart-controls/src/sections/advancedAnalytics.tsx:
##########
@@ -121,6 +121,10 @@ export const advancedAnalyticsControls: ControlPanelSectionConfig = {
['2 years ago', t('2 years ago')],
['156 weeks ago', t('156 weeks ago')],
['3 years ago', t('3 years ago')],
+ ['208 weeks ago', t('208 weeks ago')],
Review Comment:
It seems like the Y4Y comparison was removed in https://github.com/apache/superset/pull/22150, though it's not evident—per the PR description and/or comments—as to why, hence I re-added them.
--
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] feat: update time comparison choices (again) [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #26173:
URL: https://github.com/apache/superset/pull/26173#issuecomment-1840032292
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/26173?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`8644b1a`)](https://app.codecov.io/gh/apache/superset/commit/8644b1a3192ffef3d20357f76cfa1feac20e4147?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 69.21% compared to head [(`5c22c23`)](https://app.codecov.io/gh/apache/superset/pull/26173?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 69.21%.
> Report is 7 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #26173 +/- ##
=======================================
Coverage 69.21% 69.21%
=======================================
Files 1941 1943 +2
Lines 75910 75899 -11
Branches 8458 8451 -7
=======================================
- Hits 52541 52537 -4
- Misses 21174 21177 +3
+ Partials 2195 2185 -10
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/26173/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/26173/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.50% <ø> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/26173?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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] feat: update time comparison choices (again) [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #26173:
URL: https://github.com/apache/superset/pull/26173#discussion_r1416104327
##########
superset-frontend/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx:
##########
@@ -322,6 +322,10 @@ const config: ControlPanelConfig = {
['2 years', t('2 years')],
['156 weeks', t('156 weeks')],
['3 years', t('3 years')],
+ ['208 weeks ago', t('208 weeks ago')],
Review Comment:
> john-bodley Is there are reason to keep the time period in weeks when they match the year? Why not use weeks only when the number of weeks is not a multiple of 52?
In order to effectively analyze daily and weekly data and account for seasonal trends, it is essential to incorporate the weekly offset.
FYI:
Certain industries or businesses may experience specific patterns or trends on a weekly basis. For example, in the hospitality industry, weekends may have higher demand compared to weekdays. By using weeks as the time period, analysts can effectively capture these patterns and make more accurate predictions or recommendations.
--
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