-
Notifications
You must be signed in to change notification settings - Fork 357
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
DateRange: Highlight secondary range in DateRange component (web) #3910
base: master
Are you sure you want to change the base?
DateRange: Highlight secondary range in DateRange component (web) #3910
Conversation
✅ Deploy Preview for gestalt ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Looks good - mostly need a better solution for the classnames and testing
|
||
/* secondary range styles */ | ||
|
||
:global ._gestalt .react-datepicker__day--in-range-secondary { |
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.
any reason these are global? If we support custom classnames, can we just pass the classname
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 are right, we don't need the global here 👍
const datesArray = startDate && endDate ? getDatesArray(startDate, endDate) : []; | ||
return [ | ||
{ | ||
'react-datepicker__day--in-range-secondary': datesArray, |
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 might be able to pull in styles like:
import styles from 'daterange.css`
{`[styles.days-in-secondary]`:dateArray}
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 you also add a snapshot test that says secondaryHighlight range appears when selected
so we can have good coverage
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.
sure 👍🏾
return dates; | ||
} | ||
|
||
function generateHighligths( |
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.
maybe typo - should be generateHighlights
@@ -95,9 +141,17 @@ const InternalDatePickerWithForwardRef = forwardRef<HTMLInputElement, ModifiedPr | |||
previousMonthButtonLabel={ | |||
<Icon accessibilityLabel="" color="default" icon="arrow-back" size={16} /> | |||
} | |||
selected={rangeStartDate ?? undefined} | |||
selected={ | |||
selectedRange === DateRangeType.Primary |
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.
Why do we need this now, and we didn't have it earlier?
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 is because the logic for supporting both ranges (primary and secondary) was handled in the DateRange.tsx
component itself, but now we also need to know which one is selected inside the InternalDatePicker in order to apply the correct styles to the "inactive range". Previously this component only had access to the "active" range.
highlightDates={initRangeHighlight ? [initRangeHighlight] : []} | ||
highlightDates={ | ||
selectedRange === DateRangeType.Primary | ||
? generateHighligths(secondaryRangeStartDate, secondaryRangeEndDate) |
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.
Ah so you need the inverse of the selection for the highlights?
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.
Yes, here we want to highlight the "inactive" range, so, if the "active" range is the primary
, then we want to highlight the secondary, and vice versa.
: secondaryDateValue?.endDate | ||
} | ||
rangeStartDate={ | ||
selectedRange === DateRangeType.Primary |
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.
Why does this move to the Internal component 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.
For this same reason: #3910 (comment)
Summary
What changed?
When the secondary date range is enabled in the DateRange component, the inactive range will be highlighted so it makes it easier for the user to see the current selected dates.
Why?
Without the highlights, it is considered a big gap of experience since it's hard to visualize the current selected ranges, the user can only see one of them (the one is currently being selected). If we show both ranges at the same time it will clearer to the user which dates are being compared/used.
For this we use the prop
highlightDates
which allows you to pass an array of dates and a custom class name to style the dates inside the array. Link to the docs here.Screenshots
Screen.Recording.2024-12-11.at.4.08.07.p.m.mov
Screen.Recording.2024-12-11.at.4.09.59.p.m.mov
Links
Checklist