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

fix: update values for default timezone selector #17124

Merged
merged 3 commits into from
Oct 18, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,18 @@
*/
import React from 'react';
import moment from 'moment-timezone';
import { render } from 'spec/helpers/testing-library';
import { render, screen } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import TimezoneSelector from './index';

describe('TimezoneSelector', () => {
let timezone: string;
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(
Expand All @@ -36,6 +40,27 @@ describe('TimezoneSelector', () => {
);
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}
/>,
);

const select = screen.getByRole('combobox', {
name: 'Timezone Selector',
});
expect(select).toBeInTheDocument();
userEvent.click(select);
const selection = await screen.findByTitle(
'GMT -06:00 (Mountain Daylight Time)',
);
expect(selection).toBeInTheDocument();
userEvent.click(selection);
expect(selection).toBeVisible();
});
it('renders a TimezoneSelector with the closest value if passed in', async () => {
render(
<TimezoneSelector
Expand Down
33 changes: 20 additions & 13 deletions superset-frontend/src/components/TimezoneSelector/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,16 @@
* under the License.
*/

import React, { useEffect, useRef } from 'react';
import React, { useEffect, useRef, useCallback } from 'react';
import moment from 'moment-timezone';
import { t } from '@superset-ui/core';
import { Select } from 'src/components';

const DEFAULT_TIMEZONE = 'GMT Standard Time';
const DEFAULT_TIMEZONE = {
name: 'GMT Standard Time',
value: 'Africa/Abidjan',
Copy link
Member

Choose a reason for hiding this comment

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

Can we use GMT here instead? It's an official IANA time zone identifier.

If not, it might be good to say here that the Africa/Abidjan timezone has no daylight saving, so it will be identical to GMT all year long.

Copy link
Member Author

@eschutho eschutho Oct 15, 2021

Choose a reason for hiding this comment

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

Gotcha, the reason that this value is used is because when we dedupe the time zone list, we choose the first value, which happens to be this one. The user never sees the value, just the label, and it has the same exact time zone match as GMT (no daylight savings time). Do you think this might pose a problem somewhere? Many of the other time zones do the same thing. PT for example is actually Vancouver.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. I think it's fine. Maybe leave a comment saying that timezones are deduped by the first alphabetical value?

};

const MIN_SELECT_WIDTH = '400px';

const offsetsToName = {
Expand All @@ -37,7 +41,7 @@ const offsetsToName = {
'-540-480': ['Alaska Standard Time', 'Alaska Daylight Time'],
'-600-600': ['Hawaii Standard Time', 'Hawaii Daylight Time'],
'60120': ['Central European Time', 'Central European Daylight Time'],
'00': [DEFAULT_TIMEZONE, DEFAULT_TIMEZONE],
'00': [DEFAULT_TIMEZONE.name, DEFAULT_TIMEZONE.name],
'060': ['GMT Standard Time - London', 'British Summer Time'],
};

Expand Down Expand Up @@ -96,28 +100,31 @@ 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 || DEFAULT_TIMEZONE.value;

const updateTimezone = (tz: string) => {
// update the ref to track changes
prevTimezone.current = tz;
// the parent component contains the state for the value
onTimezoneChange(tz);
};
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],
);

useEffect(() => {
const updatedTz = matchTimezoneToOptions(timezone || moment.tz.guess());
if (prevTimezone.current !== updatedTz) {
updateTimezone(updatedTz);
}
}, [timezone]);
}, [timezone, updateTimezone]);

return (
<Select
ariaLabel={t('Timezone')}
ariaLabel={t('Timezone Selector')}
Copy link
Member

Choose a reason for hiding this comment

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

Super nit. I think we agreed that the convention should be Timezone selector

Suggested change
ariaLabel={t('Timezone Selector')}
ariaLabel={t('Timezone selector')}

Copy link
Member Author

@eschutho eschutho Oct 15, 2021

Choose a reason for hiding this comment

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

thank you for pointing that out. 👍

css={{ minWidth: MIN_SELECT_WIDTH }} // smallest size for current values
onChange={onTimezoneChange}
value={timezone || DEFAULT_TIMEZONE}
value={timezone || DEFAULT_TIMEZONE.value}
options={TIMEZONE_OPTIONS}
/>
);
Expand Down