-
Notifications
You must be signed in to change notification settings - Fork 236
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
Zoom improvements #269
base: master
Are you sure you want to change the base?
Zoom improvements #269
Conversation
Added zoom to domain function. This allows to zoom to a given domain programmatically. Added id in config, reason for this is that when connecting two EventDrops chart, it is very handy to have the id in the svg element the zoom is happening in.
Added the option to restrict panning (dragging behaviour) to the initial date range. So if the date range is from 2010 to 2012, it is not possible to pan to say 2009. If the minimumScale is less than 1, it is possible to zoom out, however panning will be disabled so that it is not possibel to pan to say an earlier date. ZoomToDomain function will ignore this (this is default D3 behaviour).
As we expect the dates to be already formmatted, I don't think it is neccessary to cast them to dates again.
Added optional arguments to zoomToDomain to allow for transition.
I would agree with the 1.4 release. There are several bugs fixed in the latest master that are not in 1.3 that vastly improve the user experience. Ones that I ran into before looking here were the color/radius not updating on zoom and the responsive width of the chart. I could have worked around these bugs as well if I could draw manually which would also be implemented if I'm looking at this correctly. TL;DR I'd love a 1.4 with the fixes! |
So the colour flickering has been fixed as you mentioned. Not sure what you mean by responsive width of the chart? There is already a method to redraw the chart, however, with zoomToDomain one can actually set [startDate, endDate] and zoom to this, which is very useful in a lot of cases. Think this, along with the bugfixes and vertical axis enhancement would make a great 1.4.0 |
Yes, lots of this stuff has been fixed but the fixes are not in 1.3 unless I'm mistaken. The responsive width of the chart was fixed with #249 for example, but that fix is not in 1.3 afaik. the color/radius thing was fixed with #259 but again is not in 1.3 which is the published version on npm. I'd love to see a 1.4 with the fixes since 1.3 |
Forgot to mention that there are some breaking changes with Travis CI, so updated it to work with the new xenial build environment. At the same time, I updated the node version to the latest LTS, as well as set Firefox to be the latest too. |
@jpetitcolas Hey, could you review this PR? :) |
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.
Really AWESOME! Thanks a lot! :)
I did a few comments on it. And yes, we are going to release a 1.4 version for sure. We just need to review all existing PR.
Removed id as this is simple to add manually. Chnaged to restrictPan to restrictpan so it could be referenced in MD file (if someone knows how to reference in uppercase, please change). Added in a check to see if _zoomToDomain is defined as function, if not then throw error. Renamed zoomObject to zoom as it is more consistent with other D3 example code. Zoom is now zoomFactory. Removed unused code in test for index. Changelog, configuration doc and Readme updated.
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.
Almost there! I just spotted a minor typo.
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.
md files and typo fixed.
Hi @fzaninotto, |
So here is a feature that has been requested a lot; zoom programmatically!
Changes:
Added ID in the config to be able to set this on the SVG element. I found this quite useful when zooming programmatically.Removed redundantnew Date()
in isAfter, isBefore and withingRange.Would be great if this PR could make it into master and then prepare this for a 1.4.0 release (as the bug fixes earlier are quite important in my opinion).
Closes #268, closes #260, closes #112, closes #88, closes #236, closes #168, closes #271