DOI-USGS / OWDI-Lower-Colorado-Drought-Vis

Lower Colorado Drought Visualization
Other
13 stars 12 forks source link

Began work on changes. #441

Closed Yneeb closed 9 years ago

Yneeb commented 9 years ago

This PR fixes several issues from both #438 and the review.

Review Fixes:

@jread-usgs and @lawinslow for the change "The popup “Flow” and “Average” tickers need units of measurement." is there a better abbreviation for "million acre-feet"? I'm still working on the other requested changes and hope to have them done in the next day or two.

jordansread commented 9 years ago

Looks pretty good. A few comments:

Yneeb commented 9 years ago

@jread-usgs All of the above comments have been fixed and a new change has been added.

Review Fixes:

Yneeb commented 9 years ago

@jread-usgs and @lawinslow, it has been a while since I have added any commits to this PR, so I figured I should explain what I have been working on with this. Mostly, I have been working on a suggestion made by @jread-usgs in #438 that recommended that I correct the breaks that were appearing between each of the lines. I appreciate the tip about fixing it using stroke-linejoin:round, but that attribute seems to only apply to certain elements like <polyline>. I have transitioned my code to use <polyline> instead of many <line> elements, but this has forced me to create a new way to animate the drawing of the graph. Animating with <polyline> is more difficult, but I believe I have found a way to do it correctly. This has required a large rework of the code in the script, but it will hopefully also help fix future problems. I didn't intend for this to take so long, but I hope to have some new commits to add tonight. Sorry about the delay.

jordansread commented 9 years ago

oh, sorry @Yneeb I didn't know you were doing line vs polyline.

Did you take a look at how the other polylines were animated in the app? Have you found a different solution?

Yneeb commented 9 years ago

@jread-usgs I was going to attempt something different. I'll let you know how it goes, but if it doesn't work out I'll fall back on something like the water use graph.

jordansread commented 9 years ago

Ok, good to know. That water use animation isn't supported by IE, so if you find a solution that is, we should also try it for that one. Using a key-frames CSS animation of the stroke-dash property is one option that I think would be compatible.

isuftin commented 9 years ago

IE10 and up only for CSS anims - http://caniuse.com/#search=css-animation

jordansread commented 9 years ago

yep, we aren't supporting below 10 @isuftin

isuftin commented 9 years ago

@jread-usgs I'm excited to see how CSS anims turns out.

Yneeb commented 9 years ago

@jread-usgs My way seems to work. I used some JavaScript to animate each section of the line. The script is all squished into one line, so if you want to take a look at how it works, you may want to copy it out. It appears to work in IE 11, but I assume it will work almost universally as it is just JavaScript. Let me know what you think.

438 Fixes:

jordansread commented 9 years ago

:+1: