-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add a date_option (format string) parameter to the choropleth timeline slider #2003
Add a date_option (format string) parameter to the choropleth timeline slider #2003
Conversation
4c13121
to
92d8456
Compare
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.
Looks good! Two comments that are more questions/suggestions on how to improve, but not critical.
@@ -19,6 +19,8 @@ class TimeSliderChoropleth(JSCSSMixin, Layer): | |||
styledict: dict | |||
A dictionary where the keys are the geojson feature ids and the values are | |||
dicts of `{time: style_options_dict}` | |||
date_options: str, default "ddd MMM DD YYYY" |
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.
later on it's called date_format
, why not call it that here as well?
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.
To be consistent with the other plugins. timestamped_geojson
and timeline
also use date_options
. I would prefer date_format
for the other plugins as well, but I don't think it is worth the effort.
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.
That makes sense 👍
@@ -19,6 +19,8 @@ class TimeSliderChoropleth(JSCSSMixin, Layer): | |||
styledict: dict | |||
A dictionary where the keys are the geojson feature ids and the values are | |||
dicts of `{time: style_options_dict}` | |||
date_options: str, default "ddd MMM DD YYYY" | |||
A format string to render the currently active time in the control. |
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 we specify what formatting language is being used here? Like, what fields like MMM DD are available?
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.
If you feel strongly about it, I can add a link to the formatting library used. However, I don't think it is necessary. The time string formatting language is fairly intuitive (and fairly universal). Also, I would like the documentation to be consistent between the different plugins.
Closes #1997