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

[EuiSuperDatePicker] Allow passing canRoundRelativeUnits={false}, which turns off relative unit rounding #7502

Merged
merged 5 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/upcoming/7502.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Updated `EuiSuperDatePicker` with a new `preferLargerRelativeUnits` prop, which defaults to true (current behavior). To preserve displaying the unit that users select, set this to false.
7 changes: 7 additions & 0 deletions src-docs/src/views/super_date_picker/playground.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ export const superDatePickerConfig = () => {
value: true,
};

propsToUse.preferLargerRelativeUnits = {
...propsToUse.preferLargerRelativeUnits,
type: PropTypes.Boolean,
defaultValue: true,
value: true,
};

propsToUse.locale = {
...propsToUse.locale,
type: PropTypes.String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export interface EuiDatePopoverButtonProps {
onPopoverClose: EuiPopoverProps['closePopover'];
onPopoverToggle: MouseEventHandler<HTMLButtonElement>;
position: 'start' | 'end';
preferLargerRelativeUnits?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

This prop name might be a bit unclear. What do you think of renaming it to relativeUnitRounding as in the title of this PR? :D I think including the word "rounding" makes it easier to search for when you see that rounding behavior and want to do something about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that does sound way better. Let's go with that!

Copy link
Contributor Author

@cee-chen cee-chen Feb 2, 2024

Choose a reason for hiding this comment

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

Actually, hm, does making it a verb sound better to you at all, out of curiosity? roundRelativeUnits={false} vs relativeUnitRounding={false}?

Copy link
Contributor Author

@cee-chen cee-chen Feb 2, 2024

Choose a reason for hiding this comment

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

Or yet another thought: if we want to follow is/has/can/shouldX naming convention style for booleans, we could do canRoundRelativeUnits, but now we're getting back into the original verbose var name territory lmao

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To recap our Slack discussion: We went with canRoundRelativeUnits for clearer boolean naming. roundRelativeUnits as a verb feels like it would take a callback rather than a boolean, so we avoided that (unless we someday want to do one of those union types that Tomasz hates so much, like Function | boolean 🤣)

Also our next datepicker component will definitely be named EuiSuperDuperDatePicker.

roundUp?: boolean;
timeFormat: string;
value: string;
Expand All @@ -56,6 +57,7 @@ export const EuiDatePopoverButton: FunctionComponent<
needsUpdating,
value,
buttonProps,
preferLargerRelativeUnits,
roundUp,
onChange,
locale,
Expand All @@ -82,12 +84,11 @@ export const EuiDatePopoverButton: FunctionComponent<
},
]);

const formattedValue = useFormatTimeString(
value,
dateFormat,
const formattedValue = useFormatTimeString(value, dateFormat, {
roundUp,
locale
);
locale,
preferLargerRelativeUnits,
});
let title = formattedValue;

const invalidTitle = useEuiI18n(
Expand Down Expand Up @@ -133,6 +134,7 @@ export const EuiDatePopoverButton: FunctionComponent<
<EuiDatePopoverContent
value={value}
roundUp={roundUp}
preferLargerRelativeUnits={preferLargerRelativeUnits}
onChange={onChange}
dateFormat={dateFormat}
timeFormat={timeFormat}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { LocaleSpecifier } from 'moment'; // eslint-disable-line import/named
export interface EuiDatePopoverContentProps {
value: string;
onChange: (date: string) => void;
preferLargerRelativeUnits?: boolean;
roundUp?: boolean;
dateFormat: string;
timeFormat: string;
Expand All @@ -41,6 +42,7 @@ export const EuiDatePopoverContent: FunctionComponent<
EuiDatePopoverContentProps
> = ({
value,
preferLargerRelativeUnits = true,
roundUp = false,
onChange,
dateFormat,
Expand Down Expand Up @@ -108,7 +110,9 @@ export const EuiDatePopoverContent: FunctionComponent<
<EuiRelativeTab
dateFormat={dateFormat}
locale={locale}
value={toAbsoluteString(value, roundUp)}
value={
preferLargerRelativeUnits ? toAbsoluteString(value, roundUp) : value
}
onChange={onChange}
roundUp={roundUp}
position={position}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
usePrettyDuration,
PrettyDuration,
showPrettyDuration,
useFormatTimeString,
} from './pretty_duration';

const dateFormat = 'MMMM Do YYYY, HH:mm:ss.SSS';
Expand Down Expand Up @@ -123,3 +124,53 @@ describe('showPrettyDuration', () => {
).toBe(false);
});
});

describe('useFormatTimeString', () => {
it('it takes a time string and formats it into a humanized date', () => {
expect(
renderHook(() => useFormatTimeString('now-3s', dateFormat)).result.current
).toEqual('~ a few seconds ago');
expect(
renderHook(() => useFormatTimeString('now+1m', dateFormat)).result.current
).toEqual('~ in a minute');
expect(
renderHook(() => useFormatTimeString('now+100w', dateFormat)).result
.current
).toEqual('~ in 2 years');
});

it("always parses the 'now' string as-is", () => {
expect(
renderHook(() => useFormatTimeString('now', dateFormat)).result.current
).toEqual('now');
});

describe('options', () => {
test('locale', () => {
expect(
renderHook(() =>
useFormatTimeString('now+15m', dateFormat, { locale: 'ja' })
).result.current
).toBe('~ 15分後');
});

describe('preferLargerRelativeUnits', () => {
const option = { preferLargerRelativeUnits: false };

it("allows skipping moment.fromNow()'s default rounding", () => {
expect(
renderHook(() => useFormatTimeString('now-3s', dateFormat, option))
.result.current
).toEqual('3 seconds ago');
expect(
renderHook(() => useFormatTimeString('now+1m', dateFormat, option))
.result.current
).toEqual('in a minute');
expect(
renderHook(() => useFormatTimeString('now+100w', dateFormat, option))
.result.current
).toEqual('in 100 weeks');
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import React from 'react';
import dateMath from '@elastic/datemath';
import moment, { LocaleSpecifier } from 'moment'; // eslint-disable-line import/named
import moment, { LocaleSpecifier, RelativeTimeKey } from 'moment'; // eslint-disable-line import/named
import { useEuiI18n } from '../../i18n';
import { getDateMode, DATE_MODES } from './date_modes';
import { parseRelativeParts } from './relative_utils';
Expand Down Expand Up @@ -146,9 +146,18 @@ const ISO_FORMAT = 'YYYY-MM-DDTHH:mm:ss.SSSZ';
export const useFormatTimeString = (
timeString: string,
dateFormat: string,
roundUp = false,
locale: LocaleSpecifier = 'en'
options?: {
locale?: LocaleSpecifier;
roundUp?: boolean;
preferLargerRelativeUnits?: boolean;
}
): string => {
const {
locale = 'en',
roundUp = false,
preferLargerRelativeUnits = true,
} = options || {};

// i18n'd strings
const nowDisplay = useEuiI18n('euiPrettyDuration.now', 'now');
const invalidDateDisplay = useEuiI18n(
Expand All @@ -171,7 +180,27 @@ export const useFormatTimeString = (
}

if (moment.isMoment(tryParse)) {
return `~ ${tryParse.locale(locale).fromNow()}`;
if (preferLargerRelativeUnits) {
return `~ ${tryParse.locale(locale).fromNow()}`;
} else {
// To force a specific unit to be used, we need to skip moment.fromNow()
// entirely and write our own custom moment formatted output.
const { count, unit: _unit } = parseRelativeParts(timeString);
const isFuture = _unit.endsWith('+');
const unit = isFuture ? _unit.slice(0, -1) : _unit; // We want just the unit letter without the trailing +

// @see https://momentjs.com/docs/#/customization/relative-time/
const relativeUnitKey = (
count === 1 ? unit : unit + unit
) as RelativeTimeKey;

// @see https://momentjs.com/docs/#/i18n/locale-data/
return moment.localeData().pastFuture(
isFuture ? count : count * -1,
moment.localeData().relativeTime(count, false, relativeUnitKey, false)
// Booleans don't seem to actually matter for output, .pastFuture() handles that
);
}
}

return timeString;
Expand Down Expand Up @@ -246,7 +275,7 @@ export const usePrettyDuration = ({
* If it's none of the above, display basic fallback copy
*/
const displayFrom = useFormatTimeString(timeFrom, dateFormat);
const displayTo = useFormatTimeString(timeTo, dateFormat, true);
const displayTo = useFormatTimeString(timeTo, dateFormat, { roundUp: true });
const fallbackDuration = useEuiI18n(
'euiPrettyDuration.fallbackDuration',
'{displayFrom} to {displayTo}',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ const findInternalInstance = (
};

describe('EuiSuperDatePicker', () => {
// RTL doesn't automatically clean up portals/datepicker popovers between tests
afterEach(() => {
const portals = document.querySelectorAll('[data-euiportal]');
portals.forEach((portal) => portal.parentNode?.removeChild(portal));
});

shouldRenderCustomStyles(<EuiSuperDatePicker onTimeChange={noop} />, {
skip: { style: true },
});
Expand Down Expand Up @@ -313,5 +319,59 @@ describe('EuiSuperDatePicker', () => {
expect(container.firstChild).toMatchSnapshot();
});
});

describe('preferLargerRelativeUnits', () => {
const props = {
onTimeChange: noop,
start: 'now-300m',
end: 'now',
};

it('defaults to true, which will round relative units up to the next largest unit', () => {
const { getByTestSubject } = render(
<EuiSuperDatePicker {...props} preferLargerRelativeUnits={true} />
);
fireEvent.click(getByTestSubject('superDatePickerShowDatesButton'));

const startButton = getByTestSubject(
'superDatePickerstartDatePopoverButton'
);
expect(startButton).toHaveTextContent('~ 5 hours ago');

const countInput = getByTestSubject(
'superDatePickerRelativeDateInputNumber'
);
expect(countInput).toHaveValue(5);

const unitSelect = getByTestSubject(
'superDatePickerRelativeDateInputUnitSelector'
);
expect(unitSelect).toHaveValue('h');

fireEvent.change(countInput, { target: { value: 300 } });
fireEvent.change(unitSelect, { target: { value: 'd' } });
expect(startButton).toHaveTextContent('~ 10 months ago');
});

it('when false, allows preserving the unit set in the start/end time timestamp', () => {
const { getByTestSubject } = render(
<EuiSuperDatePicker {...props} preferLargerRelativeUnits={false} />
);
fireEvent.click(getByTestSubject('superDatePickerShowDatesButton'));

const startButton = getByTestSubject(
'superDatePickerstartDatePopoverButton'
);
expect(startButton).toHaveTextContent('300 minutes ago');

const unitSelect = getByTestSubject(
'superDatePickerRelativeDateInputUnitSelector'
);
expect(unitSelect).toHaveValue('m');

fireEvent.change(unitSelect, { target: { value: 'd' } });
expect(startButton).toHaveTextContent('300 days ago');
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,15 @@ export type EuiSuperDatePickerProps = CommonProps & {
* Props passed to the update button #EuiSuperUpdateButtonProps
*/
updateButtonProps?: EuiSuperUpdateButtonProps;

/**
* By default, relative units will be rounded up to next largest unit of time
* (for example, 90 minutes will become ~ 2 hours).
*
* If you do not want this behavior and instead wish to keep the exact units
* input by the user, set this flag to `false`.
*/
preferLargerRelativeUnits?: boolean;
};

type EuiSuperDatePickerInternalProps = EuiSuperDatePickerProps & {
Expand Down Expand Up @@ -241,6 +250,7 @@ export class EuiSuperDatePickerInternal extends Component<
recentlyUsedRanges: [],
refreshInterval: 1000,
showUpdateButton: true,
preferLargerRelativeUnits: true,
start: 'now-15m',
timeFormat: 'HH:mm',
width: 'restricted',
Expand Down Expand Up @@ -468,6 +478,7 @@ export class EuiSuperDatePickerInternal extends Component<
isQuickSelectOnly,
showUpdateButton,
commonlyUsedRanges,
preferLargerRelativeUnits,
timeOptions,
dateFormat,
refreshInterval,
Expand Down Expand Up @@ -562,6 +573,7 @@ export class EuiSuperDatePickerInternal extends Component<
utcOffset={utcOffset}
timeFormat={timeFormat}
locale={locale || contextLocale}
preferLargerRelativeUnits={preferLargerRelativeUnits}
isOpen={this.state.isStartDatePopoverOpen}
onPopoverToggle={this.onStartDatePopoverToggle}
onPopoverClose={this.onStartDatePopoverClose}
Expand All @@ -582,6 +594,7 @@ export class EuiSuperDatePickerInternal extends Component<
utcOffset={utcOffset}
timeFormat={timeFormat}
locale={locale || contextLocale}
preferLargerRelativeUnits={preferLargerRelativeUnits}
roundUp
isOpen={this.state.isEndDatePopoverOpen}
onPopoverToggle={this.onEndDatePopoverToggle}
Expand Down
Loading