Skip to content

feat: update time comparison choices (again)#26173

Closed
john-bodley wants to merge 1 commit into
apache:masterfrom
john-bodley:john-bodley--the-before-times
Closed

feat: update time comparison choices (again)#26173
john-bodley wants to merge 1 commit into
apache:masterfrom
john-bodley:john-bodley--the-before-times

Conversation

@john-bodley
Copy link
Copy Markdown
Member

SUMMARY

Similar to #22458, this PR adds Y5Y time comparison choices to make it faster to compare with pre-pandemic times.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@john-bodley john-bodley force-pushed the john-bodley--the-before-times branch from 5f9822b to a6a7987 Compare December 5, 2023 03:49
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the Y4Y comparison was removed in #22150, though it's not evident—per the PR description and/or comments—as to why, hence I re-added them.

@john-bodley john-bodley force-pushed the john-bodley--the-before-times branch from a6a7987 to 5c22c23 Compare December 5, 2023 04:54
@john-bodley john-bodley marked this pull request as ready for review December 5, 2023 04:55
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8644b1a) 69.21% compared to head (5c22c23) 69.21%.
Report is 7 commits behind head on master.

Additional details and impacted files
@@           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 Coverage Δ
javascript 56.50% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

['2 years', t('2 years')],
['156 weeks', t('156 weeks')],
['3 years', t('3 years')],
['208 weeks ago', t('208 weeks ago')],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@justinpark justinpark Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Y4Y comparison was removed in #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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @justinpark for the additional context. @john-bodley I wonder if we should make these configurable somehow.

@john-bodley
Copy link
Copy Markdown
Member Author

Closing in favor of implementing an internal custom override. It seems like Y4Y, Y5Y, etc. comparisons are very much organization dependent.

@john-bodley john-bodley closed this Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants