-
Notifications
You must be signed in to change notification settings - Fork 144
Conversation
This is looking really good @psealock - so just wondering if we should consider using the WP core date picker here - apologies for not mentioning that as a suggestion on the original issue: While the design is a bit different, it has passed the a11y review from core. @LevinMedia thoughts from the design side of things? |
@timmyc @psealock re: the question about the core component... could we re-style so it looks more like the input we have now, and behaves like what I describe at #1097 in regards to using number input, and automatically advancing through each part of the input... while maintaining the accessibility? |
Here is core's DatePicker with some basic Woo styling: We should go in this direction. It will involve some functionality upstream pull requests (invalid days, and firstDayOfWeek support) and some stylistic changes (day sizing and box v. circle). wc-admin's date range picker is heavily tied to its input format (single text input v. GB's multiple inputs) so we can't reuse Core's datepicker there, but we could match styles.
I really like the suggestion by @LevinMedia in #1097. Happily, Core has separated the inputs from the picker for us, so we can deal with it separately. I think there can be some a11y gains to be made with that approach. I'll explore those thoughts further in that issue. @LevinMedia how do you feel about the styles in the screen shot and moving towards this direction with the date-range picker for reports? |
65970f8
to
4b6fbda
Compare
07be925 switches to use Core's Datepicker. Here are upstream Gutenberg changes needed to get to parity. Note that these features are not required for this PR to be merged.
These seem to be reasonable requests and I think I'll be able to make them happen. |
Confirmed with @LevinMedia that this is the direction we'll want to pursue. I'm going to mark this as blocked for now until the above^^^ Gutenberg issues can be resolved. |
@timmyc how do you feel about getting this merged ahead of pending Gutenberg fixes? The only one that affects usability is WordPress/gutenberg#12855 which leaves the experience broken for keyboard users if unmerged.
Doing so would be on the assumption that these get merged, but allow the continuation of work on filters. In the worst case, we can swap out GB components retrospectively. |
4b6fbda
to
12bec92
Compare
dateFormat={ dateFormat } | ||
invalidDays="none" | ||
onUpdate={ onDatePickerUpdate } | ||
invalidDays="future" |
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.
note to self: invalidDays
and onUpdate
are duplicated
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.
One minor comment in the code, but testing out nicely. Verified all seems well with current datepicker usage. I'm fine with landing this before the gberg PRs are merged myself.
Working towards #1027
Depends on WordPress/gutenberg#12855
Test
psealock:add/focusOnMount-to-dropdown
branch of Gutenberg or just make the two line change in Dropdown: add focusOnMount prop to pass onto Popover component WordPress/gutenberg#12855./wp-admin/admin.php?page=wc-admin#/devdocs/calendar
block
, the fix for that is here DateTimePicker: fix prop warning for WordPress/gutenberg#12933Key Decisions