You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/03/10 01:44:07 UTC
[GitHub] [superset] ktmud commented on a change in pull request #19090: refactor(TimezoneSelector): simplify override logics and tests
ktmud commented on a change in pull request #19090:
URL: https://github.com/apache/superset/pull/19090#discussion_r823257632
##########
File path: superset-frontend/src/components/TimezoneSelector/index.tsx
##########
@@ -92,43 +92,40 @@ const TIMEZONE_OPTIONS = TIMEZONES.map(zone => ({
timezoneName: zone.name,
}));
+const TIMEZONE_OPTIONS_SORT_COMPARATOR = (
+ a: typeof TIMEZONE_OPTIONS[number],
+ b: typeof TIMEZONE_OPTIONS[number],
+) =>
+ moment.tz(currentDate, a.timezoneName).utcOffset() -
+ moment.tz(currentDate, b.timezoneName).utcOffset();
+
+TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARATOR);
Review comment:
#19085 requires select options to be pre-sorted.
##########
File path: superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx
##########
@@ -18,60 +18,78 @@
*/
import React from 'react';
import moment from 'moment-timezone';
-import { render, screen } from 'spec/helpers/testing-library';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import TimezoneSelector from './index';
-describe('TimezoneSelector', () => {
- let timezone: string | undefined;
- const onTimezoneChange = jest.fn(zone => {
- timezone = zone;
- });
- beforeEach(() => {
- timezone = undefined;
- });
- it('renders a TimezoneSelector with a default if undefined', () => {
- jest.spyOn(moment.tz, 'guess').mockReturnValue('America/New_York');
- render(
- <TimezoneSelector
- onTimezoneChange={onTimezoneChange}
- timezone={timezone}
- />,
- );
- expect(onTimezoneChange).toHaveBeenCalledWith('America/Nassau');
- });
- it('should properly select values from the offsetsToName map', async () => {
- jest.spyOn(moment.tz, 'guess').mockReturnValue('America/New_York');
- render(
- <TimezoneSelector
- onTimezoneChange={onTimezoneChange}
- timezone={timezone}
- />,
- );
+jest.spyOn(moment.tz, 'guess').mockReturnValue('America/New_York');
- const select = screen.getByRole('combobox', {
- name: 'Timezone selector',
- });
- expect(select).toBeInTheDocument();
- userEvent.click(select);
+const getSelectOptions = () =>
+ waitFor(() => document.querySelectorAll('.ant-select-item-option-content'));
- const isDaylight = moment('now').isDST();
- let findTitle = 'GMT -07:00 (Mountain Standard Time)';
- if (isDaylight) {
- findTitle = 'GMT -06:00 (Mountain Daylight Time)';
- }
- const selection = await screen.findByTitle(findTitle);
- expect(selection).toBeInTheDocument();
- userEvent.click(selection);
- expect(selection).toBeVisible();
- });
- it('renders a TimezoneSelector with the closest value if passed in', async () => {
- render(
- <TimezoneSelector
- onTimezoneChange={onTimezoneChange}
- timezone="America/Los_Angeles"
- />,
- );
- expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Vancouver');
+it('use the timezone from `moment` if no timezone provided', () => {
+ const onTimezoneChange = jest.fn();
+ render(<TimezoneSelector onTimezoneChange={onTimezoneChange} />);
+ expect(onTimezoneChange).toHaveBeenCalledTimes(1);
+ expect(onTimezoneChange).toHaveBeenCalledWith('America/Nassau');
+});
+
+it('update to closest deduped timezone when timezone is provided', async () => {
+ const onTimezoneChange = jest.fn();
+ render(
+ <TimezoneSelector
+ onTimezoneChange={onTimezoneChange}
+ timezone="America/Los_Angeles"
+ />,
+ );
+ expect(onTimezoneChange).toHaveBeenCalledTimes(1);
+ expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Vancouver');
+});
+
+it('use the default timezone when an invalid timezone is provided', async () => {
+ const onTimezoneChange = jest.fn();
+ render(
+ <TimezoneSelector onTimezoneChange={onTimezoneChange} timezone="UTC" />,
+ );
+ expect(onTimezoneChange).toHaveBeenCalledTimes(1);
+ expect(onTimezoneChange).toHaveBeenLastCalledWith('Africa/Abidjan');
+});
+
+it('can select a timezone values and returns canonical value', async () => {
+ const onTimezoneChange = jest.fn();
+ render(
+ <TimezoneSelector
+ onTimezoneChange={onTimezoneChange}
+ timezone="America/Nassau"
+ />,
+ );
+
+ const searchInput = screen.getByRole('combobox', {
+ name: 'Timezone selector',
});
+ expect(searchInput).toBeInTheDocument();
+
+ userEvent.click(searchInput);
+ const options = await getSelectOptions();
+ // select option is first
+ expect(options[0]).toHaveTextContent('GMT -05:00 (Eastern Standard Time)');
+ // others are ranked by offset
+ expect(options[1]).toHaveTextContent('GMT -11:00 (Pacific/Pago_Pago)');
+ expect(options[2]).toHaveTextContent('GMT -10:00 (Hawaii Standard Time)');
+ expect(options[3]).toHaveTextContent('GMT -10:00 (America/Adak)');
+
+ // search for mountain time
+ await userEvent.type(searchInput, 'mou', { delay: 10 });
+
+ const isDaylight = moment(moment.now()).isDST();
+
+ let findTitle = 'GMT -07:00 (Mountain Standard Time)';
+ if (isDaylight) {
+ findTitle = 'GMT -06:00 (Mountain Daylight Time)';
+ }
+ const selectOption = await screen.findByTitle(findTitle);
+ expect(selectOption).toBeInTheDocument();
+ userEvent.click(selectOption);
+ expect(onTimezoneChange).toHaveBeenCalledTimes(1);
+ expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Cambridge_Bay');
Review comment:
Test the updated timezone value after selecting an option
##########
File path: superset-frontend/src/components/TimezoneSelector/index.tsx
##########
@@ -92,43 +92,40 @@ const TIMEZONE_OPTIONS = TIMEZONES.map(zone => ({
timezoneName: zone.name,
}));
+const TIMEZONE_OPTIONS_SORT_COMPARATOR = (
+ a: typeof TIMEZONE_OPTIONS[number],
+ b: typeof TIMEZONE_OPTIONS[number],
+) =>
+ moment.tz(currentDate, a.timezoneName).utcOffset() -
+ moment.tz(currentDate, b.timezoneName).utcOffset();
+
+TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARATOR);
+
+const matchTimezoneToOptions = (timezone: string) =>
+ TIMEZONE_OPTIONS.find(option => option.offsets === getOffsetKey(timezone))
+ ?.value || DEFAULT_TIMEZONE.value;
+
const TimezoneSelector = ({ onTimezoneChange, timezone }: TimezoneProps) => {
- const prevTimezone = useRef(timezone);
- const matchTimezoneToOptions = (timezone: string) =>
- TIMEZONE_OPTIONS.find(option => option.offsets === getOffsetKey(timezone))
- ?.value || DEFAULT_TIMEZONE.value;
-
- const updateTimezone = useCallback(
- (tz: string) => {
- // update the ref to track changes
- prevTimezone.current = tz;
- // the parent component contains the state for the value
- onTimezoneChange(tz);
- },
- [onTimezoneChange],
+ const validTimezone = useMemo(
+ () => matchTimezoneToOptions(timezone || moment.tz.guess()),
+ [timezone],
);
+ // force trigger a timezone update if provided `timezone` is not invalid
useEffect(() => {
- const updatedTz = matchTimezoneToOptions(timezone || moment.tz.guess());
- if (prevTimezone.current !== updatedTz) {
- updateTimezone(updatedTz);
+ if (timezone !== validTimezone) {
+ onTimezoneChange(validTimezone);
}
- }, [timezone, updateTimezone]);
+ }, [validTimezone, onTimezoneChange, timezone]);
return (
<Select
ariaLabel={t('Timezone selector')}
css={{ minWidth: MIN_SELECT_WIDTH }} // smallest size for current values
- onChange={onTimezoneChange}
- value={timezone || DEFAULT_TIMEZONE.value}
Review comment:
Make sure `value` is a valid timezone. `timezone` may be different than `updatedTz`.
##########
File path: superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx
##########
@@ -18,60 +18,78 @@
*/
import React from 'react';
import moment from 'moment-timezone';
-import { render, screen } from 'spec/helpers/testing-library';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import TimezoneSelector from './index';
-describe('TimezoneSelector', () => {
- let timezone: string | undefined;
- const onTimezoneChange = jest.fn(zone => {
- timezone = zone;
- });
- beforeEach(() => {
- timezone = undefined;
- });
- it('renders a TimezoneSelector with a default if undefined', () => {
- jest.spyOn(moment.tz, 'guess').mockReturnValue('America/New_York');
- render(
- <TimezoneSelector
- onTimezoneChange={onTimezoneChange}
- timezone={timezone}
- />,
- );
- expect(onTimezoneChange).toHaveBeenCalledWith('America/Nassau');
- });
- it('should properly select values from the offsetsToName map', async () => {
- jest.spyOn(moment.tz, 'guess').mockReturnValue('America/New_York');
- render(
- <TimezoneSelector
- onTimezoneChange={onTimezoneChange}
- timezone={timezone}
- />,
- );
+jest.spyOn(moment.tz, 'guess').mockReturnValue('America/New_York');
- const select = screen.getByRole('combobox', {
- name: 'Timezone selector',
- });
- expect(select).toBeInTheDocument();
- userEvent.click(select);
+const getSelectOptions = () =>
+ waitFor(() => document.querySelectorAll('.ant-select-item-option-content'));
- const isDaylight = moment('now').isDST();
- let findTitle = 'GMT -07:00 (Mountain Standard Time)';
- if (isDaylight) {
- findTitle = 'GMT -06:00 (Mountain Daylight Time)';
- }
- const selection = await screen.findByTitle(findTitle);
- expect(selection).toBeInTheDocument();
- userEvent.click(selection);
- expect(selection).toBeVisible();
- });
- it('renders a TimezoneSelector with the closest value if passed in', async () => {
- render(
- <TimezoneSelector
- onTimezoneChange={onTimezoneChange}
- timezone="America/Los_Angeles"
- />,
- );
- expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Vancouver');
+it('use the timezone from `moment` if no timezone provided', () => {
+ const onTimezoneChange = jest.fn();
+ render(<TimezoneSelector onTimezoneChange={onTimezoneChange} />);
+ expect(onTimezoneChange).toHaveBeenCalledTimes(1);
+ expect(onTimezoneChange).toHaveBeenCalledWith('America/Nassau');
+});
+
+it('update to closest deduped timezone when timezone is provided', async () => {
+ const onTimezoneChange = jest.fn();
+ render(
+ <TimezoneSelector
+ onTimezoneChange={onTimezoneChange}
+ timezone="America/Los_Angeles"
+ />,
+ );
+ expect(onTimezoneChange).toHaveBeenCalledTimes(1);
+ expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Vancouver');
+});
+
+it('use the default timezone when an invalid timezone is provided', async () => {
+ const onTimezoneChange = jest.fn();
+ render(
+ <TimezoneSelector onTimezoneChange={onTimezoneChange} timezone="GMT" />,
+ );
+ expect(onTimezoneChange).toHaveBeenCalledTimes(1);
+ expect(onTimezoneChange).toHaveBeenLastCalledWith('Africa/Abidjan');
+});
+
+it('Can select a timezone values and returns canonical value', async () => {
+ const onTimezoneChange = jest.fn();
+ render(
+ <TimezoneSelector
+ onTimezoneChange={onTimezoneChange}
+ timezone="America/Nassau"
+ />,
+ );
+
+ const searchInput = screen.getByRole('combobox', {
+ name: 'Timezone selector',
});
+ expect(searchInput).toBeInTheDocument();
+
+ userEvent.click(searchInput);
+ const options = await getSelectOptions();
+ // select option is first
+ expect(options[0]).toHaveTextContent('GMT -05:00 (Eastern Standard Time)');
+ // others are ranked by offset
+ expect(options[1]).toHaveTextContent('GMT -11:00 (Pacific/Pago_Pago)');
+ expect(options[2]).toHaveTextContent('GMT -10:00 (Hawaii Standard Time)');
+ expect(options[3]).toHaveTextContent('GMT -10:00 (America/Adak)');
Review comment:
New tests for option ordering.
##########
File path: superset-frontend/src/components/TimezoneSelector/index.tsx
##########
@@ -92,43 +92,40 @@ const TIMEZONE_OPTIONS = TIMEZONES.map(zone => ({
timezoneName: zone.name,
}));
+const TIMEZONE_OPTIONS_SORT_COMPARATOR = (
+ a: typeof TIMEZONE_OPTIONS[number],
+ b: typeof TIMEZONE_OPTIONS[number],
+) =>
+ moment.tz(currentDate, a.timezoneName).utcOffset() -
+ moment.tz(currentDate, b.timezoneName).utcOffset();
+
+TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARATOR);
+
+const matchTimezoneToOptions = (timezone: string) =>
+ TIMEZONE_OPTIONS.find(option => option.offsets === getOffsetKey(timezone))
+ ?.value || DEFAULT_TIMEZONE.value;
+
const TimezoneSelector = ({ onTimezoneChange, timezone }: TimezoneProps) => {
- const prevTimezone = useRef(timezone);
- const matchTimezoneToOptions = (timezone: string) =>
- TIMEZONE_OPTIONS.find(option => option.offsets === getOffsetKey(timezone))
- ?.value || DEFAULT_TIMEZONE.value;
-
- const updateTimezone = useCallback(
- (tz: string) => {
- // update the ref to track changes
- prevTimezone.current = tz;
- // the parent component contains the state for the value
- onTimezoneChange(tz);
- },
- [onTimezoneChange],
+ const validTimezone = useMemo(
+ () => matchTimezoneToOptions(timezone || moment.tz.guess()),
+ [timezone],
Review comment:
Changed from `useRef` to simply triggering a setState in `useEffect`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org