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 binding for amp-date-picker min attribute #19035
Conversation
43cd16c
to
611c438
Compare
@@ -53,7 +53,7 @@ export function withDatePickerCommon(WrappedComponent) { | |||
*/ | |||
function isOutsideRange(min, max, date) { | |||
const maxInclusive = max && moment(max); | |||
const minInclusive = min && moment(min); | |||
const minInclusive = min ? moment(min) : getDefaultMinDate(max); |
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.
shouldn't this be getDefaultMinDate(min)
?
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.
No, but the method name is somewhat confusing. The default minimum date depends on what the maximum date is. The parameter is named max in the function definition to help clarify
function getDefaultMinDate(max) { |
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.
Bind-related changes LGTM.
@@ -173,6 +173,9 @@ attr_lists: { | |||
blacklisted_value_regex: "__amp_source_origin" | |||
} | |||
attrs: { name: "week-day-format" } | |||
# amp-bind | |||
attrs: { name: "[max]" } | |||
attrs: { name: "[min]" } |
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.
@honeybadgerdontcare FYI validator change.
@@ -33,7 +33,7 @@ export function withDatePickerCommon(WrappedComponent) { | |||
const moment = requireExternal('moment'); | |||
|
|||
/** | |||
* @param {!moment} max | |||
* @param {string} max |
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.
can this stay {!moment}
if you pass moment(max)
from isOutsideRange
?
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.
Sure, that could eliminate the extra call to moment
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.
validation changes lgtm
62d5485
to
942e247
Compare
942e247
to
9631878
Compare
- cl/219385362 Revision bump for #19035
* Add binding for amp-date-picker min attribute * Clarify getDefaultMinDate behavior and remove extra moment call
- cl/219385362 Revision bump for ampproject#19035
Fixes #16410
Implements, documents and tests validation of
amp-bind
bindings for the attributesmin
andmax
onamp-date-picker
./to @nainar
/cc @choumx