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
Added time slider, time layer aware HOC #314
Conversation
Always updates the TIME parameter without checking the services constraints.
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.
Great addition! I really like the HOC.
I just added some minor (non-blocking) comments you might want to address.
package.json
Outdated
@@ -119,6 +119,7 @@ | |||
"metalsmith-layouts": "1.8.1", | |||
"metalsmith-markdown": "0.2.1", | |||
"minami": "1.2.3", | |||
"moment": "2.20.1", |
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.
Should be a dependency
.
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.
Done.
src/Slider/TimeSlider.jsx
Outdated
|
||
/** | ||
* The default value(s). | ||
* @type {any} |
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.
Is there any other type than Array<String>
or String
supported?
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.
Nope, docs adapted.
src/Slider/TimeSlider.jsx
Outdated
|
||
/** | ||
* The current value(s). | ||
* @type {any} |
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.
See above.
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.
Nope, docs adapted.
src/Slider/TimeSlider.jsx
Outdated
* @return {String} the formatted timestamps | ||
*/ | ||
formatTimestamp = unix => { | ||
return moment(unix * 1000).format('DD.MM. HH:mm'); |
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.
Please make format string configurable.
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.
Done.
Adds a time slider that operates on ISO 8601 time strings. Also adds a HOC that updates layers based on the current time setting.
@terrestris/devs please review