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(events-stream): Add zoom back to charts (APP-832) #10904

Merged
merged 8 commits into from Dec 6, 2018

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Dec 4, 2018

This adds zoom back to events chart -- icons are a WIP (I'm hoping we can come back to it in a followup), but behavior has changed so that we can start in "zoom-enabled" state which I think greatly improves the zoom behavior.

There's also a lot of logic so that react does not re-render our chart component (and allows eCharts to handle the zoom). This provides a more seamless experience when zooming on the chart.

The buttons are a distraction but zoom doesn't work unless that toolbar is present. A potential solution is to change their icons to a 1px img so that they are "hidden".

preview

@billyvg billyvg changed the title feat(events-stream): Add zoom back to charts feat(events-stream): Add zoom back to charts (APP-832) Dec 4, 2018
@billyvg billyvg force-pushed the feat/events-stream/add-zoom-back branch from a54dc20 to 260fbed Compare December 4, 2018 17:51
@billyvg billyvg requested a review from a team December 4, 2018 21:39
Copy link
Member

@lynnagara lynnagara left a comment

Choose a reason for hiding this comment

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

A couple things from testing the UI

  • chart doesn't render at all on initial load
  • zooming in on a date range does not update the date picker with hour/minute

@billyvg
Copy link
Member Author

billyvg commented Dec 5, 2018

@lynnagara Thanks, the last rebase with master broke the initial load (and also uncovered other subtle bugs that have been fixed too).

Things to look out for with manual tests:

  • Chart zooming functions should not re-render chart, but should re-render the table
  • Changing parameters using the header should re-render both
  • There's a special case in the charts to handle "last 24 hours" (or a period of <= 1 day) where the x-axis of the chart becomes an interval of 1 hour.
    • Known oddness: If you zoom into an interval of ~1 day, it won't re-render w/ hourly intervals (I think that's ok for now).

Copy link
Member

@lynnagara lynnagara left a comment

Choose a reason for hiding this comment

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

I think it would be good to hide the icons (1px image hack is totally fine if that's all we can easily do). Since zoom is effectively an alternate method of date selection input it's odd to think about "restoring" - what does that mean and to what state do we consider the "original"?

@billyvg
Copy link
Member Author

billyvg commented Dec 6, 2018

We'll followup with a design/ux pass on this. Zooming w/ the chart has a nice animation with it and the "original" is the state since the last chart component render. Since users will have no concept of this, it'll lead to some confusing UX.

@billyvg billyvg merged commit d4f2534 into master Dec 6, 2018
@billyvg billyvg deleted the feat/events-stream/add-zoom-back branch December 6, 2018 17:55
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants