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

Zoom improvements #269

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

Busteren
Copy link
Contributor

@Busteren Busteren commented May 15, 2019

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.
  • ZoomToDomain function. Takes in a domain and zooms to it. Ignores any restrictions to scale or translate (as default by D3). There are also optional arguments so it is possible to do transitions to have a neat effect. I did not manage to test the function itself, as when I tried, it says that SVGElement is unknown🤷🏼‍♂️
  • Restrict panning. Default false as not to break backwards compatibility. If this is set, one cannot pan outside the start and end ranges (or what is set by default). If minimumScale on zoom is less than 1, you can zoom out, however, you won't be able to pan
  • Removed redundant new 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

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.
@jaredkirkley
Copy link

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!

@Busteren
Copy link
Contributor Author

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
Also think travis/dependencies might need an update.

@jaredkirkley
Copy link

jaredkirkley commented May 15, 2019

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
Great work all you contributors btw!

@Busteren
Copy link
Contributor Author

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.

@Busteren
Copy link
Contributor Author

@jpetitcolas Hey, could you review this PR? :)

Copy link
Contributor

@jpetitcolas jpetitcolas left a 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.

.travis.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.spec.js Outdated Show resolved Hide resolved
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.
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
src/isAfter.js Outdated Show resolved Hide resolved
src/isBefore.js Outdated Show resolved Hide resolved
src/withinRange.js Outdated Show resolved Hide resolved
src/zoom.js Outdated Show resolved Hide resolved
Copy link
Member

@fzaninotto fzaninotto left a 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.

docs/configuration.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Busteren Busteren left a 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.

@Busteren
Copy link
Contributor Author

Hi @fzaninotto,
I have made the changes (even though it didn't flag them as resolved for some reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants