Skip to content

Conversation

@bbovenzi
Copy link
Contributor

@bbovenzi bbovenzi commented May 5, 2021

Add a timezone select dropdown to the new UI. Closes #14807

This is a frontend only toggle, it does not change any backend Airflow config.

Also includes a custom implementation of react-select that uses the Chakra theme and component library.

Screen Shot 2021-05-05 at 11 34 39 AM


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

bbovenzi added 4 commits May 4, 2021 13:13
Copied the UX from the current UI on how to change timezone preferences.

Working on a Chakra UI wrapper for React-Select.
@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label May 5, 2021
@ryanahamilton
Copy link
Contributor

I'm wondering if the dropdown values should have the offset values in parens after the name? Seems like there should be a correlation between the selection and what is displayed up top as the current selection e.g. (-08:00)

Copy link
Contributor

@ryanahamilton ryanahamilton left a comment

Choose a reason for hiding this comment

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

I'm going to have to take another look tomorrow, but this is looking really nice! The MultiSelect component is quite a feat!

RecursiveCSSObject,
CSSWithMultiValues,
} from '@chakra-ui/react';
import { ChevronDownIcon } from '@chakra-ui/icons';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should just continue utilizing react-icons instead of adding a new dependency.

Suggested change
import { ChevronDownIcon } from '@chakra-ui/icons';
import { FiChevronDown } from 'react-icons/fi';

@@ -0,0 +1,259 @@
/*!
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this file name to MultiSelect.tsx to match the function name and differentiate from a standard <select>?


const toastDuration = 3000;
const refetchInterval = isTest ? false : 1000;
const refetchInterval = false;
Copy link
Member

Choose a reason for hiding this comment

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

?

import tz from 'dayjs/plugin/timezone';
import Select from 'components/MultiSelect';

import timezones from 'utils/timezones.json';
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather we used a third party library for this than having to maintain our own list.

dateTime={now.toString()}
fontSize="md"
>
{now.format('h:mmA Z')}
Copy link
Member

Choose a reason for hiding this comment

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

So here's the thing about localization: the "common" way dates are displayed in about half the world is to use the 24 hour clock

https://en.wikipedia.org/wiki/Date_and_time_representation_by_country#/media/File:12_24_Hours_World_Map.svg

This is possibly an extra feature/PR as I guess the time display is a per-user setting, not a property of what timezone is selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do a 12/24hr toggle as a separate pr right after this.


// guess timezone on browser or default to utc and don't guess when testing
const isTest = process.env.NODE_ENV === 'test';
const [timezone, setTimezone] = useState((!isTest && dayjs.tz.guess()) || 'UTC');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [timezone, setTimezone] = useState((!isTest && dayjs.tz.guess()) || 'UTC');
const [timezone, setTimezone] = useState(isTest ? 'UTC' : dayjs.tz.guess());

@bbovenzi
Copy link
Contributor Author

bbovenzi commented May 6, 2021

Updated with a 3rd party timezone library.

Screen Shot 2021-05-06 at 11 06 32

const { timezone, setTimezone } = useTimezoneContext();
const [now, setNow] = useState(dayjs().tz(timezone));

const timezones = getTimeZones();
Copy link
Member

Choose a reason for hiding this comment

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

Did you see this section of the tzdb docs https://github.com/vvo/tzdb/#notes:

const value = timeZones.find((timeZone) => {
  return dbData.timeZone === timeZone.name || timeZone.group.includes(dbData.timeZone);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Should I include all group names in the dropdown too?

Copy link
Member

Choose a reason for hiding this comment

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

I'll let you decide -- but I think that might be a bit noisy/large?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I thought too. I'll leave it as-is

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Looks good to me, but React is not my strong suit :D

Copy link
Contributor

@ryanahamilton ryanahamilton left a comment

Choose a reason for hiding this comment

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

Very nice!

@ryanahamilton ryanahamilton merged commit 3614910 into apache:master May 7, 2021
@ryanahamilton ryanahamilton deleted the timezone-select branch May 7, 2021 17:49
@ashb ashb added the AIP-38 Modern Web Application label Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-38 Modern Web Application area:UI Related to UI/UX. For Frontend Developers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Design/build timezone switcher modal

3 participants