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

ui: Combine relative and absolute range picker #4777

Merged
merged 14 commits into from
Jun 20, 2024
Merged

Conversation

yomete
Copy link
Contributor

@yomete yomete commented Jun 18, 2024

This PR combines the relative and absolute range pickers to be in a tab switcher so as to make it easier to switch between both range pickers.

Resolves #4748

Copy link

alwaysmeticulous bot commented Jun 18, 2024

🤖 Meticulous spotted visual differences in 368 of 488 screens tested: view and approve differences detected.

Last updated for commit e1c1ecd. This comment will update as new commits are pushed.

@pre-commit-ci-lite pre-commit-ci-lite bot requested a review from a team as a code owner June 18, 2024 11:01
onRangeSelection,
}: DateTimeRangePickerTextProps): JSX.Element => {
const isRelativeRange = range.from.isRelative();
const dateString = `${formatDateStringForUI(range.from)} → ${formatDateStringForUI(range.to)}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can try something like

getRangeStringForUI(): string {
if (this.from.isRelative() && this.to.isRelative() && (this.to as RelativeDate).value === 0) {
const from = this.from as RelativeDate;
return `Last ${from.value} ${from.unit}${from.value > 1 ? 's' : ''}`;
}
const formattedFrom = formatDateStringForUI(this.from);
const formattedTo = formatDateStringForUI(this.to)
.replace(getUtcStringForDate(this.from as AbsoluteDate, 'll'), '')
.trim();
return `${formattedFrom}${formattedTo}`;
}
here so that the text is a bit concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this is better, thanks for the suggestion

return (
<div className="flex flex-col gap-3 items-center text-sm p-4">
{presetRanges.map(({value, unit, text}) => (
<Button
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These buttons are rendered as primary variants, should we go with neutral as default and only set primary for the selected range?

Having all the buttons as primary looks a bit too busy.
Screenshot 2024-06-19 at 10 19 23 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm!

Copy link
Contributor

@manojVivek manojVivek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment to fix the width changes when switching around the tabs, otherwise lgtm!

@yomete yomete merged commit 02ec867 into main Jun 20, 2024
38 checks passed
@yomete yomete deleted the combine-date-pickers branch June 20, 2024 11:12
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.

Combine relative and absolute range picker
3 participants