Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(explore): Add time shift color control to ECharts #29897

Merged

Conversation

rtexelm
Copy link
Member

@rtexelm rtexelm commented Aug 8, 2024

SUMMARY

Adds a new checkbox control to timeseries capable charts allowing the user to choose whether or not to duplicate the original series colors for the shifts.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

new-time-shift-color.mov

TESTING INSTRUCTIONS

  1. Create a new chart using the Sales dataset.
  2. Use COUNT(*) as your metric.
  3. Set order_date as the X-Axis and a Month time grain.
  4. Apply any time range filter that returns data.
  5. Expand the time comparison and set 1 month ago.
  6. Click on CREATE CHART.
  7. Navigate to Customize tab
  8. Click the Time Shift color checkbox

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

@dosubot dosubot bot added change:frontend Requires changing the frontend explore:design Related to the Explore UI/UX viz:charts:echarts Related to Echarts labels Aug 8, 2024
@yousoph
Copy link
Member

yousoph commented Aug 8, 2024

/testenv up

Copy link
Contributor

github-actions bot commented Aug 9, 2024

@yousoph Ephemeral environment spinning up at http://35.90.51.45:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@michael-s-molina
Copy link
Member

The only concern that I have with this approach is about charts consumption at the dashboard layer. It might be confusing for users that time shifts are represented differently among charts which might harm UX. I'm not sure if we should discuss more about this as you can see in #26041

@michael-s-molina
Copy link
Member

michael-s-molina commented Aug 12, 2024

Another option is to assume that all time shifts will be of varied colors and remove the need for this checkbox. Essentially, not make the correlation between series and their time shifts using colors, only the legend.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 13, 2024
@yousoph
Copy link
Member

yousoph commented Aug 13, 2024

I'm not too concerned about the dashboard level - I'm guessing that there will be consistency at the org or creator level with this control based on preference so there aren't likely to be too many discrepancies at the dashboard level.

In terms of defaulting to use varied colors, I'm not opposed to that but I thought the original fix in #24048 was to retain the same color for the time shift as the original control. Since that was the fix that was implemented, we thought adding a control would be the best way to give the flexibility to use either option.

@michael-s-molina
Copy link
Member

I'm not too concerned about the dashboard level - I'm guessing that there will be consistency at the org or creator level with this control based on preference so there aren't likely to be too many discrepancies at the dashboard level.

This is a good point @yousoph. We have other controls that might provoke the same effect if not used consistently. Thank you for the additional context 👍🏼

@rusackas rusackas requested a review from eschutho August 15, 2024 19:05
@fpassantino
Copy link

Hello, any news about this feature? It's really important for us. Thanks

Copy link
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@rtexelm rtexelm merged commit c5594f2 into apache:master Sep 12, 2024
33 checks passed
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rtexelm
Copy link
Member Author

rtexelm commented Sep 12, 2024

@fpassantino sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend explore:design Related to the Explore UI/UX packages plugins size/L viz:charts:echarts Related to Echarts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants