-
Notifications
You must be signed in to change notification settings - Fork 704
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
new(vx-brush): add initialBrushPosition #618
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR @maryschmidt ! I think a window
event
listener is a reasonable approach, but have one major suggested change.
One other thought is that we need to prevent duplicate invocations of |
Sounds good, I'll update the PR |
this one won't make |
Sorry for the delay on this. Things have been absolutely crazy lately... After some thought, I believe we can address the vast majority of use cases for #577 by allowing users to provide a starting range for the selected area controller (I'm calling it Thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maryschmidt I agree that this this is a good way to support #577.
overall looks great but had a couple of suggestions esp re naming 😄 then we can get this puppy 🐶 merged!
}; | ||
|
||
componentWillUnmount = () => { | ||
window.removeEventListener('mouseup', this.handleDragEnd); | ||
}; | ||
|
||
componentDidUpdate(prevProps: BaseBrushProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ideally we would update state
if the initialBrushState
updates, but don't want to add a ton of checks in this method. I guess for now users could force re-render with a new component key
🤔 not sure if @hshoff has thoughts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tough one... I've been thinking about this as well.
For now, I think pairing key
with initialBrushState
makes the most sense in this context. - It avoids the hazards of getting into "semi-controlled component" territory where users would re-default initial values but the component would otherwise manage its own logic.
- The behavior would be consistent with conventions around setting default values on other input elements (dropdowns, date pickers, HTML inputs, etc.)
The downside I see is this wouldn't support animating between changes. My own immediate use case for this is for linking to different regions of a chart, so that would definitely be nice to have, but like you said, I would like to avoid adding checks everywhere...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once we update the constructor
syntax. thanks for iterating on this, it's going to be super useful!
@emeeks is waiting for it! 💖
Updated! Thanks for the feedback 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woot! not sure CI actually ran but will merge and fix that separately if need be
Awesome, thanks for the contribution @maryschmidt! |
sorry for the delay in the release, I'm testing this now and it doesn't really work :( will have a fix out soon / then release. |
out in |
💥 Breaking Changes
🚀 Enhancements
📝 Documentation
🐛 Bug Fix
🏠 Internal