-
Notifications
You must be signed in to change notification settings - Fork 90
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 date range to time range dropdown #763
Conversation
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #763 +/- ##
==========================================
+ Coverage 91.53% 91.56% +0.03%
==========================================
Files 88 89 +1
Lines 2350 2359 +9
Branches 443 443
==========================================
+ Hits 2151 2160 +9
Misses 176 176
Partials 23 23 ☔ View full report in Codecov by Sentry. |
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 Sumair!
This looks great and works fine !
However here are a few changes that are IMO needed before we can land this :)
Tell me what you think!
src/__tests__/Search/__snapshots__/CompareOverTime.test.tsx.snap
Outdated
Show resolved
Hide resolved
… MUI theme's text.seondary
Thank you @julienw for the very high-quality and thorough review! 💯 |
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, this is much better!
It would be good to change also the text displayed in the results view, when the edit mode isn't active. This happens in SearchOverTime
line 157
{timeRangeText} |
Can you look at it?
I also left some other nits to the existing code, tell me what you think
{text} | ||
<Box | ||
sx={{ color: 'text.secondary' }} | ||
className={`${FontSize.Small}`} |
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.
nit: FontSize.Small is just a string (it's a class name), so use it simply as a string
className={`${FontSize.Small}`} | |
className={FontSize.Small} |
const options: Intl.DateTimeFormatOptions = { | ||
dateStyle: 'medium', | ||
}; | ||
const dateFormatter = new Intl.DateTimeFormat('en-US', options); |
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.
These options do not change for different renders, so you can define them outside of the component, so that they're defined just once. They're quite expensive to instanciate.
Just create a toplevel:
const DATE_FORMATTER = new Intl.DateTimeFormat('en-US', { dateStyle: 'medium' });
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.
Because you'll have to use the same date formatter in SearchOverTime (see above), it would be a good idea to move the format part to another file. I'd suggest a new file src/utils/format.js
.
In it you could have:
const DATE_FORMATTER = new Intl.DateTimeFormat('en-US', { dateStyle: 'medium' });
export function formatDate(date: Date) {
return dateFormatter.format(date);
}
export function formatDateRange(date1: Date, date2: Date) {
return formatDate(date1) + " - " + formatDate(date2);
}
And then from TimeRangeDropdown
you could use formatDateRange
and just add the parentheses and the extra space.
In the future formatDateRange
could be improved to not display useless things (maybe), for example when the same year we wouldn't display it. But don't bother with that now.
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.
Great idea!
I went ahead and moved the formatting logic into src/utils/format.ts
as suggested. This will definitely help keep things clean and reusable.
Thanks for the feedback! 👍
> | ||
{text} | ||
<div className={styles.dateRange}> |
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.
you don't need the extra div, just add the space-between
to the MenuItem directly.
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.
Adding space-between
directly to MenuItem
does the job well for menu items only and leads to unexpected behavior for SelectedItem
, as the children do not render as flexbox items. This is because MenuItem
has display: flex
set by default while SelectedItem
does not.
SelectedItem |
MenuItem |
---|---|
![]() |
![]() |
To maintain consistent flexbox behavior across both MenuItems and SelectedItem
, I applied the following workaround:
1- Wrapped content within a flex container: Enclose the content of MenuItem within a flex container (i.e. a div with display: 'flex'
and justifyContent: 'space-between'
).
2- Disabled display: flex
on MenuItem by setting display: block
on MenuItem
to override its default flex display.
These changes ensure that both MenuItems
and SelectedItem
are displayed as flexbox items, preserving the intended layout and spacing.
Alertnatively, we could ditch the flex behaviour altogether and keep them inline only like this.
What do you think ?
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 explanation, let's keep it like you did it, but please add a comment before line 47 with this explanation:
// This extra flexible div is needed, so that the information looks the same in
// both the MenuItem (in the dropdown options) and the SelectedItem (in
// the dropdown, when the dropdown is closed).
@julienw
|
I'd go with the no space version :-) thanks for asking |
export function formatDateRange(date1: Date, date2: Date) { | ||
return formatDate(date1) + ' - ' + formatDate(date2); | ||
} |
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.
you know what, I just realized that Intl.DateTimeFormat knows how to format ranges (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/formatRange).
But I'm not sure we want that in this context.
From my test, the best formatter would be:
const dateRangeFormatter = new Intl.DateTimeFormat('en-US', {
month: "short",
day: "numeric"
});
// then
export function formatDateRange(date1: Date, date2: Date) {
return dateRangeFormatter.formatRange(date1, date2);
}
By not specifying the year, it would show up only when needed.
Can you please try that and see how this looks?
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.
@julienw
For comparison, here is how it looks:
month: "short", day: "numeric" | dateStyle: 'medium' |
---|---|
![]() |
![]() |
I like how concise this format is while still conveying the complete information.
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.
Yeah, I'd like the concise format better if this was showing up separate from the others.
But in this case, I find the format at the right easier to parse visually, because the same operation is present at the same location for every rows.
So let's keep dateStyle: medium
, but thanks for trying it!
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 looks good to me with the extra comment, thanks!
You'll need to rebase and regenerate the snapshots as well, because they conflict with a recent commit.
[esanuandra] Reduce margins for search form (#773) [esanuandra] PCF-425 Add colors and icons for the confidence information (#764) [Sumair Qaisar] Add date range to time range dropdown (#763) [beatrice-acasandrei] PCF-296 Remove interval param when comparing with a base (#792) [esanuandra] PCF-394 Remove the "compare with base" image aria-label "two overlapping circles" and make it aria-hidden=true (#791) [beatrice-acasandrei] Add missing information to the Subtests page (#789)
Solves Bug 1922027
Description
This PR enhances the existing time range dropdown in the CompareOverTime UI by displaying specific dates alongside the predefined time range options. This provides users with better clarity on the exact date ranges they are selecting.
Changes:
Updated the time range dropdown to display specific start and end dates next to predefined options (e.g., "Last 7 days" now shows the exact dates).
Screenshot: