Skip to content
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

Merged
merged 15 commits into from
Oct 24, 2024
Merged

Conversation

sumairq
Copy link
Collaborator

@sumairq sumairq commented Oct 2, 2024

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:

image

Copy link

netlify bot commented Oct 2, 2024

Deploy Preview for mozilla-perfcompare ready!

Name Link
🔨 Latest commit 3c581e1
🔍 Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/67196d47764bb6000861b2ca
😎 Deploy Preview https://deploy-preview-763--mozilla-perfcompare.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.56%. Comparing base (01b30d1) to head (3c581e1).
Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@julienw julienw left a 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!

@sumairq sumairq requested a review from julienw October 14, 2024 17:25
@sumairq
Copy link
Collaborator Author

sumairq commented Oct 14, 2024

Thank you @julienw for the very high-quality and thorough review! 💯
I am learning a lot from your feedback, and it has improved not only the code but also my thinking process. 🧠 🙌

Copy link
Contributor

@julienw julienw left a 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

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}`}
Copy link
Contributor

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

Suggested change
className={`${FontSize.Small}`}
className={FontSize.Small}

Comment on lines 60 to 63
const options: Intl.DateTimeFormatOptions = {
dateStyle: 'medium',
};
const dateFormatter = new Intl.DateTimeFormat('en-US', options);
Copy link
Contributor

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' });

Copy link
Contributor

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.

Copy link
Collaborator Author

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}>
Copy link
Contributor

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.

Copy link
Collaborator Author

@sumairq sumairq Oct 17, 2024

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
selectItem dropdownItems

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.
image

What do you think ?

Copy link
Contributor

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).

@sumairq
Copy link
Collaborator Author

sumairq commented Oct 17, 2024

@julienw
Another question :
Do you think a space between paranthesis and the Date Ranges looks better than no space ?

Space No Space
image image

@sumairq sumairq requested a review from julienw October 17, 2024 21:06
@julienw
Copy link
Contributor

julienw commented Oct 18, 2024

@julienw Another question : Do you think a space between paranthesis and the Date Ranges looks better than no space ?
Space No Space
image image

I'd go with the no space version :-) thanks for asking

Comment on lines +9 to +11
export function formatDateRange(date1: Date, date2: Date) {
return formatDate(date1) + ' - ' + formatDate(date2);
}
Copy link
Contributor

@julienw julienw Oct 18, 2024

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?

Copy link
Collaborator Author

@sumairq sumairq Oct 18, 2024

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'
image image

I like how concise this format is while still conveying the complete information.

Copy link
Contributor

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!

Copy link
Contributor

@julienw julienw left a 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.

@julienw julienw merged commit 3f7d028 into mozilla:main Oct 24, 2024
10 checks passed
@julienw julienw mentioned this pull request Nov 5, 2024
julienw added a commit that referenced this pull request Nov 5, 2024
[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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants