LG-4621: Add zooming to Chart
#2561
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 6bb2d93 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +577 B (+0.04%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
stephl3
reviewed
Nov 27, 2024
| if (onChartReady) { | ||
| chartInstance.on('finished', onChartReady); | ||
| function onInitialRender() { | ||
| if (zoomable) { |
There was a problem hiding this comment.
should this check that onZoomSelect is also passed? otherwise, clicking zoom doesn't do anything right?
or alternatively, should there be a default zoom behavior that takes place? (also I could be misunderstanding the onZoomSelect. I'm going off of the fact that when I click zoom in the storybook example, it doesn't seem to do anything)
There was a problem hiding this comment.
This was something that was discussed in #b-team last week here. We decided zoomable would turn on the "zoom effect" where one can select an area, and that onZoomSelect would still be needed for any zooming to occur. This is because after speaking with Gregg and Intel, it seemed there was little value in having a default zoom behavior, as zooming should always require a refetch.
The alternative would be to have onZoomSelect implicitly also turn on the "zoom effect", since as you noted, it won't actually zoom in on the current data and the handler is essentially required. This was a bit debated so I'm definitely open to other thoughts if this seems confusing.
There was a problem hiding this comment.
Discussed offline: After you dropped Adam and I just decided that we are going to get rid of zoomable altogether. We don't want to set a precedent with the pattern of implying functionality (like the click and drag functionality) based on the presence of a handler like this. However this feels like an exception to the rule due to the fact that the data is essentially always controlled and there never exists a scenario where one is useful without the other.
Will push an update shortly and re-request a review then!
|
@stephl3 Just a note, I also removed the change to storybook since it seemed like having a prop that did nothing but trigger the call of a handler would only lead to confusion. |
stephl3
approved these changes
Dec 3, 2024
| | Name | Description | Type | Default | | ||
| | --------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------ | ------- | | ||
| | `onChartReady` | Callback to be called when chart is finished rendering. | `() => void` | | | ||
| | `onZoomSelect` _(optional)_ | Callback to be called when a user clicks and drags on a chart to zoom. Click and drag action will only be enabled if this handler is present. | `(ZoomSelectionEvent) => void` | | |
There was a problem hiding this comment.
nit: didn't think about this previously but could be helpful to capture what a ZoomSelectionEvent is somewhere in the readme
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
Adds zooming functionality to
Chart🎟 Jira ticket: [LG-4621](https://jira.mongodb.org/browse/LG-4621/
✅ Checklist
For bug fixes, new features & breaking changes
yarn changesetand documented my changesFor new components
🧪 How to test changes
Try in Storybook