-
Notifications
You must be signed in to change notification settings - Fork 2k
Hosting Dashboard: Add Date Range Controls #105235
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
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
…ck - see PR description for more
<InputControl | ||
value={ label } | ||
onChange={ () => {} } |
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 implementation is not accessible for a few reasons:
- It's not accessibly labeled (a
Tooltip
only adds visual information). - A screen reader user will not be able to tell that the input has button-like handlers.
The first is easily fixable but the second is more of a fundamental problem. It should semantically be a button
. Any reason we're bending over backwards to use an input
element? It doesn't quite make sense to me even from a visual standpoint, since the text is not editable.
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.
Thanks for digging in to this, and for these suggestions.
No, no reason for it to specifically be InputControl
, other than it made sense to me at the time. The design specs suggested a TextControl
which made less sense, but to me the logical next step was an InputControl
. But button
does make a lot more sense - I've updated this.
<TextControl | ||
placeholder="YYYY-MM-DD" |
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.
How about we use a type="date"
? The hardcoded "YYYY-MM-DD"
is not great for i18n either.
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.
Thanks for the suggestion -- updated!
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.
Some additional feedback on top of @mirka 's:
Overall, I recommend taking a look at this Storybook example which already addresses many items of feedback left so far (ie. using a button for the dropdown trigger, using |
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.
Thank you for working on this! I haven't tested the UI yet, but leaving some code-related comments.
gmtOffset: s?.gmt_offset ?? 0, | ||
timezoneString: s?.timezone_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.
It seems unfortunate that sometimes we have to use gmtOffset
and sometimes timezoneString
😬 Do you have any info on when one is defined over the other? Maybe I'm misreading the code, but I'm interpreting it as it will either be one or the other.
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.
I'm glad you asked! I've got a follow-up task for the calendar to add various improvements (including for eg. storybook, additional tests beyond those in datetime), and I added a note to look into the timezone functionality more.
It was a head scratcher for me, and as best I can tell from the research I've done gmt_offset
will always exist (defaults to 0 if not set), but timezone_string
can be empty when the site uses a numeric offset.
date_i18n
apparently has issues with certain timezone settings such as numerical ones, which means that using timezone_string
where possible should fix issues in those cases (eg. DST) - IANA zones (e.g., Europe/London) apparently embed DST transition rules.
As I understand, using 'TZDate' as suggested in the calendar package code doesn't help with this case anyway, though I've added it where possible.
For now I think some good testing across different timezones would be helpful, for the 'MVP' anyway.
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.
Wow, today I learned how wild the return value of wp_timezone_string()
is!
… different child components, based on feedback
@mirka @ciampo @p-jackson thanks so much for the reviews here. I should have addressed all the feedback with the new commits (or with comments to suggestions) at this stage. I also have a follow-on task for more refinements with this Date Range Picker - including a story book example, more tests, and more research into the date and time functionality itself. Initially this was to be an MVP with the minimal date range picker functionality as needed, for follow up later, but the example shared in Slack inspired me to do as much as possible for the MVP so it has become a lot more complex than I anticipated. For new feedback it would be good to see what can be included in a follow-up, or what we should fix for an MVP. Thanks again for the deep dives! |
gmtOffset: s?.gmt_offset ?? 0, | ||
timezoneString: s?.timezone_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.
Wow, today I learned how wild the return value of wp_timezone_string()
is!
<TextControl | ||
placeholder="YYYY-MM-DD" |
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.
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.
Thanks for the improvements. Seems like a good place to merge and keep iterating on the logging interface IMO
import { formatDateWithOffset } from '../../utils/datetime'; | ||
|
||
// Normalize/comparison helpers | ||
const startOfDay = ( date?: Date ) => |
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.
It seems we can consider to use date-fns for these helpers 🙂
gmtOffset, | ||
timezoneString, | ||
}: DateRangePickerProps ) { | ||
const locale = useLocale(); |
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.
As a component, it would be better not to rely on this hook. Instead, the component should accept locale through props.
const desktopLabelId = `presets-label-${ instanceId }-desktop`; | ||
|
||
const label = useMemo( | ||
() => formatLabel( start, end, locale, timezoneString, gmtOffset ), |
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 doesn't seem to be a heavy computation. Maybe we don't need to memo it 🤔
); | ||
|
||
// Reset internal draft state when key inputs change by remounting the inner component | ||
const resetKey = useMemo( |
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.
Same
compositeActiveId: string | null; | ||
setCompositeActiveId: ( id: string | null ) => void; | ||
siteToday: Date; | ||
siteTodayStr: 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.
It seems weird that there is site
concept here. Would it be better to call it today
?
justify-content: center; | ||
width: 100%; | ||
} | ||
} |
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.
We need empty line at the end of file
<span aria-hidden="true" className="daterange-input__text"> | ||
{ label } | ||
</span> | ||
<Icon icon={ calendar } size={ 24 } style={ { marginRight: 8, paddingLeft: 4 } } /> |
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 we provide the icon to button and set the position to right 🤔
style={ { marginBottom: 16 } } | ||
> | ||
<VisuallyHidden id={ mobileLabelId }>{ __( 'Date range presets' ) }</VisuallyHidden> | ||
<PresetsListbox |
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.
as="div" | ||
spacing={ 1 } | ||
className="daterange-presets" | ||
style={ { marginBottom: 16 } } |
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.
Instead of using marginBottom, would it be better to add another VStack and set the spacing 🤔
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.
Also, can we move VStack to PresetsListbox
itself? It seems the component needs it internally 🤔
: { from: fromDraft, to: toDraft }; | ||
|
||
return ( | ||
<div style={ { padding: 12 } }> |
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.
Would it be better to use a VStack here to keep the spacing consistent instead of setting margins individually? 🤔
Part of DOTCOM-14159
Proposed Changes
InputControlButton -- design specs suggested a text control but then they were also suggesting that we should use the old implementation. However after a conversation in Slack - p1755018709178949-slack-C0982082MSR - I'm using the component available from@automattic/ui
.For follow-up:
Why are these changes being made?
Testing Instructions
http://calypso.localhost:3000/v2/sites/your-site/logs
.Screenshots:
Desktop:
Mobile:
Pre-merge Checklist