-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: Filter: Add date-range to custom date range filter value component [GAUD-6578] #4764
Conversation
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
* Date/time range input type | ||
* @type {'date'|'date-time'} | ||
*/ | ||
type: { type: String } |
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.
Another option would be to have a bool like hide-time
. The advantage of type
is in case in the future we need to do some sort of time-only range or some other state (though there are currently no plans for those)
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.
Of the two, I prefer the type
approach for the reason you mentioned.
Another alternative we can contemplate is busting it into two different components, which might be overkill if the API is essentially the same and there isn't much in the way of conditional logic.
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.
Agreed - at this point (nor going forward) I don't see it as necessary to split them out.
this.startValue = e.target.startValue ? getUTCDateTimeFromLocalDateTime(e.target.startValue, '0:0') : undefined; | ||
this.endValue = e.target.endValue ? getUTCDateTimeFromLocalDateTime(e.target.endValue, '0:0') : undefined; |
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.
d2l-input-date-range
values are only localized so this gets the UTC value for midnight on the selected date.
end-value="${ifDefined(this.endValue)}" | ||
label="Custom Range" | ||
label="${this.localize('components.filter-dimension-set-date-time-range-value.text')}" |
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.
This would (potentially) be the same as the list item text if not set by the consumer. I'll be looking at accessibility in a follow up story so I'll do some testing at that point to see if that's too repetitive.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
🎉 This PR is included in version 3.18.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Jira ticket
Adds
type
property which defaults todate-time
but can be set todate
in order to use ad2l-input-date-range
component inside the custom date filter value component.