You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/05/05 15:38:12 UTC
[GitHub] [airflow] bbovenzi opened a new pull request #15674: New UI: Add Timezone select
bbovenzi opened a new pull request #15674:
URL: https://github.com/apache/airflow/pull/15674
Add a timezone select dropdown to the new UI. Closes #14804
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.
<img width="317" alt="Screen Shot 2021-05-05 at 11 34 39 AM" src="https://user-images.githubusercontent.com/4600967/117168133-81867a00-ad8d-11eb-8f23-8bc6b4e96a38.png">
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627220644
##########
File path: airflow/ui/src/providers/TimezoneProvider.tsx
##########
@@ -45,9 +45,9 @@ type Props = {
const TimezoneProvider = ({ children }: Props): ReactElement => {
// TODO: add in default_timezone when GET /ui-metadata is available
- // guess timezone on browser or default to utc
- const [timezone, setTimezone] = useState(dayjs.tz.guess() || 'UTC');
-
+ // 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');
Review comment:
```suggestion
const [timezone, setTimezone] = useState(isTest ? 'UTC' : dayjs.tz.guess());
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r628112702
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -44,9 +44,9 @@ const TimezoneDropdown: React.FC = () => {
const timezones = getTimeZones();
let currentTimezone;
- const options = timezones.map(({ name, currentTimeFormat }) => {
+ const options = timezones.map(({ name, currentTimeFormat, group }) => {
const label = `${currentTimeFormat.substring(0, 6)} ${name.replace(/_/g, ' ')}`;
- if (name === timezone) currentTimezone = { label, value: name };
+ if (name === timezone || group.includes(timezone)) currentTimezone = { label, value: name };
Review comment:
Does this let you search for "America/San Diego" and get LA for instance?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627504666
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -0,0 +1,90 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Box,
+ Button,
+ Menu,
+ MenuButton,
+ MenuList,
+ Tooltip,
+} from '@chakra-ui/react';
+import dayjs from 'dayjs';
+import tz from 'dayjs/plugin/timezone';
+import Select from 'components/MultiSelect';
+
+import timezones from 'utils/timezones.json';
+import { useTimezoneContext } from 'providers/TimezoneProvider';
+
+dayjs.extend(tz);
+
+interface Option { value: string, label: string }
+
+const TimezoneDropdown: React.FC = () => {
+ const { timezone, setTimezone } = useTimezoneContext();
+ const [now, setNow] = useState(dayjs().tz(timezone));
+
+ let currentTimezone;
+ timezones.find((group) => (
+ group.options.find((zone) => {
+ if (zone.value === timezone) {
+ currentTimezone = zone;
+ return true;
+ }
+ return false;
+ })
+ ));
+
+ const onChangeTimezone = (newTimezone: Option | null) => {
+ if (newTimezone) {
+ setTimezone(newTimezone.value);
+ setNow(dayjs().tz(newTimezone.value));
+ }
+ };
+
+ return (
+ <Menu isLazy>
+ <Tooltip label="Change time zone" hasArrow>
+ <MenuButton as={Button} variant="ghost" mr="4">
+ <Box
+ as="time"
+ dateTime={now.toString()}
+ fontSize="md"
+ >
+ {now.format('h:mmA Z')}
Review comment:
I'll do a 12/24hr toggle as a separate pr right after this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r628267734
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -44,9 +44,9 @@ const TimezoneDropdown: React.FC = () => {
const timezones = getTimeZones();
let currentTimezone;
- const options = timezones.map(({ name, currentTimeFormat }) => {
+ const options = timezones.map(({ name, currentTimeFormat, group }) => {
const label = `${currentTimeFormat.substring(0, 6)} ${name.replace(/_/g, ' ')}`;
- if (name === timezone) currentTimezone = { label, value: name };
+ if (name === timezone || group.includes(timezone)) currentTimezone = { label, value: name };
Review comment:
Got it!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#issuecomment-833598480
Updated with a 3rd party timezone library.
<img width="423" alt="Screen Shot 2021-05-06 at 11 06 32" src="https://user-images.githubusercontent.com/4600967/117321450-bf99a180-ae52-11eb-95a6-068e52b3029f.png">
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627533444
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -0,0 +1,88 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Box,
+ Button,
+ Menu,
+ MenuButton,
+ MenuList,
+ Tooltip,
+} from '@chakra-ui/react';
+import dayjs from 'dayjs';
+import tz from 'dayjs/plugin/timezone';
+import { getTimeZones } from '@vvo/tzdb';
+
+import Select from 'components/MultiSelect';
+import { useTimezoneContext } from 'providers/TimezoneProvider';
+
+dayjs.extend(tz);
+
+interface Option { value: string, label: string }
+
+const TimezoneDropdown: React.FC = () => {
+ const { timezone, setTimezone } = useTimezoneContext();
+ const [now, setNow] = useState(dayjs().tz(timezone));
+
+ const timezones = getTimeZones();
Review comment:
I'll let you decide -- but I think that might be a bit noisy/large?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627023326
##########
File path: airflow/ui/src/components/Select.tsx
##########
@@ -0,0 +1,259 @@
+/*!
Review comment:
Change this file name to `MultiSelect.tsx` to match the function name and differentiate from a standard `<select>`?
##########
File path: airflow/ui/src/components/Select.tsx
##########
@@ -0,0 +1,259 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/* eslint-disable no-underscore-dangle */
+import React from 'react';
+import Select, { components as selectComponents, Props as SelectProps } from 'react-select';
+import {
+ Flex,
+ Tag,
+ TagCloseButton,
+ TagLabel,
+ Divider,
+ CloseButton,
+ Center,
+ Box,
+ Portal,
+ StylesProvider,
+ useMultiStyleConfig,
+ useStyles,
+ useTheme,
+ useColorModeValue,
+ RecursiveCSSObject,
+ CSSWithMultiValues,
+} from '@chakra-ui/react';
+import { ChevronDownIcon } from '@chakra-ui/icons';
Review comment:
We should just continue utilizing `react-icons` instead of adding a new dependency.
```suggestion
import { FiChevronDown } from 'react-icons/fi';
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r628240258
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -0,0 +1,88 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Box,
+ Button,
+ Menu,
+ MenuButton,
+ MenuList,
+ Tooltip,
+} from '@chakra-ui/react';
+import dayjs from 'dayjs';
+import tz from 'dayjs/plugin/timezone';
+import { getTimeZones } from '@vvo/tzdb';
+
+import Select from 'components/MultiSelect';
+import { useTimezoneContext } from 'providers/TimezoneProvider';
+
+dayjs.extend(tz);
+
+interface Option { value: string, label: string }
+
+const TimezoneDropdown: React.FC = () => {
+ const { timezone, setTimezone } = useTimezoneContext();
+ const [now, setNow] = useState(dayjs().tz(timezone));
+
+ const timezones = getTimeZones();
+
+ let currentTimezone;
+ const options = timezones.map(({ name, currentTimeFormat, group }) => {
+ const label = `${currentTimeFormat.substring(0, 6)} ${name.replace(/_/g, ' ')}`;
+ if (name === timezone || group.includes(timezone)) currentTimezone = { label, value: name };
+ return { label, value: name };
+ });
+
+ const onChangeTimezone = (newTimezone: Option | null) => {
+ if (newTimezone) {
+ setTimezone(newTimezone.value);
+ setNow(dayjs().tz(newTimezone.value));
+ }
+ };
+
+ return (
+ <Menu isLazy>
+ <Tooltip label="Change time zone" hasArrow>
+ <MenuButton as={Button} variant="ghost" mr="4">
+ <Box
+ as="time"
+ dateTime={now.toString()}
+ fontSize="md"
+ >
+ {now.format('HH:mm Z')}
+ </Box>
+ </MenuButton>
+ </Tooltip>
+ <MenuList placement="top-end" minWidth="350px">
+ <Box px="3" pb="1">
Review comment:
You should be able to remove this `Box` wrapper and just move the style props to the parent `MenuList`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627508772
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -0,0 +1,88 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Box,
+ Button,
+ Menu,
+ MenuButton,
+ MenuList,
+ Tooltip,
+} from '@chakra-ui/react';
+import dayjs from 'dayjs';
+import tz from 'dayjs/plugin/timezone';
+import { getTimeZones } from '@vvo/tzdb';
+
+import Select from 'components/MultiSelect';
+import { useTimezoneContext } from 'providers/TimezoneProvider';
+
+dayjs.extend(tz);
+
+interface Option { value: string, label: string }
+
+const TimezoneDropdown: React.FC = () => {
+ const { timezone, setTimezone } = useTimezoneContext();
+ const [now, setNow] = useState(dayjs().tz(timezone));
+
+ const timezones = getTimeZones();
Review comment:
Did you see this section of the tzdb docs https://github.com/vvo/tzdb/#notes:
```js
const value = timeZones.find((timeZone) => {
return dbData.timeZone === timeZone.name || timeZone.group.includes(dbData.timeZone);
});
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627523326
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -0,0 +1,88 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Box,
+ Button,
+ Menu,
+ MenuButton,
+ MenuList,
+ Tooltip,
+} from '@chakra-ui/react';
+import dayjs from 'dayjs';
+import tz from 'dayjs/plugin/timezone';
+import { getTimeZones } from '@vvo/tzdb';
+
+import Select from 'components/MultiSelect';
+import { useTimezoneContext } from 'providers/TimezoneProvider';
+
+dayjs.extend(tz);
+
+interface Option { value: string, label: string }
+
+const TimezoneDropdown: React.FC = () => {
+ const { timezone, setTimezone } = useTimezoneContext();
+ const [now, setNow] = useState(dayjs().tz(timezone));
+
+ const timezones = getTimeZones();
Review comment:
Good catch. Should I include all group names in the dropdown too?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton merged pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ryanahamilton merged pull request #15674:
URL: https://github.com/apache/airflow/pull/15674
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627536784
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -0,0 +1,88 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Box,
+ Button,
+ Menu,
+ MenuButton,
+ MenuList,
+ Tooltip,
+} from '@chakra-ui/react';
+import dayjs from 'dayjs';
+import tz from 'dayjs/plugin/timezone';
+import { getTimeZones } from '@vvo/tzdb';
+
+import Select from 'components/MultiSelect';
+import { useTimezoneContext } from 'providers/TimezoneProvider';
+
+dayjs.extend(tz);
+
+interface Option { value: string, label: string }
+
+const TimezoneDropdown: React.FC = () => {
+ const { timezone, setTimezone } = useTimezoneContext();
+ const [now, setNow] = useState(dayjs().tz(timezone));
+
+ const timezones = getTimeZones();
Review comment:
That's what I thought too. I'll leave it as-is
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r628263747
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -44,9 +44,9 @@ const TimezoneDropdown: React.FC = () => {
const timezones = getTimeZones();
let currentTimezone;
- const options = timezones.map(({ name, currentTimeFormat }) => {
+ const options = timezones.map(({ name, currentTimeFormat, group }) => {
const label = `${currentTimeFormat.substring(0, 6)} ${name.replace(/_/g, ' ')}`;
- if (name === timezone) currentTimezone = { label, value: name };
+ if (name === timezone || group.includes(timezone)) currentTimezone = { label, value: name };
Review comment:
<img width="354" alt="Screen Shot 2021-05-07 at 10 34 21" src="https://user-images.githubusercontent.com/4600967/117465519-68a9d000-af17-11eb-958d-19c0696e9ea0.png">
If the guessed timezone is "America/San Diego" then it will resolve it to show LA instead of being null.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r628244162
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -44,9 +44,9 @@ const TimezoneDropdown: React.FC = () => {
const timezones = getTimeZones();
let currentTimezone;
- const options = timezones.map(({ name, currentTimeFormat }) => {
+ const options = timezones.map(({ name, currentTimeFormat, group }) => {
const label = `${currentTimeFormat.substring(0, 6)} ${name.replace(/_/g, ' ')}`;
- if (name === timezone) currentTimezone = { label, value: name };
+ if (name === timezone || group.includes(timezone)) currentTimezone = { label, value: name };
Review comment:
I'm not quite sure what this code is for then? If the only option is what is in the drop down, then we can ignore groups can't we?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627209373
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -0,0 +1,90 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Box,
+ Button,
+ Menu,
+ MenuButton,
+ MenuList,
+ Tooltip,
+} from '@chakra-ui/react';
+import dayjs from 'dayjs';
+import tz from 'dayjs/plugin/timezone';
+import Select from 'components/MultiSelect';
+
+import timezones from 'utils/timezones.json';
Review comment:
I'd rather we used a third party library for this than having to maintain our own list.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#issuecomment-833161245
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)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r628216707
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -44,9 +44,9 @@ const TimezoneDropdown: React.FC = () => {
const timezones = getTimeZones();
let currentTimezone;
- const options = timezones.map(({ name, currentTimeFormat }) => {
+ const options = timezones.map(({ name, currentTimeFormat, group }) => {
const label = `${currentTimeFormat.substring(0, 6)} ${name.replace(/_/g, ' ')}`;
- if (name === timezone) currentTimezone = { label, value: name };
+ if (name === timezone || group.includes(timezone)) currentTimezone = { label, value: name };
Review comment:
No, I'd need to parse out all the cities and group names for that.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627206462
##########
File path: airflow/ui/src/api/index.ts
##########
@@ -50,7 +50,7 @@ setLogger({
});
const toastDuration = 3000;
-const refetchInterval = isTest ? false : 1000;
+const refetchInterval = false;
Review comment:
?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15674: New UI: Add Timezone select
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15674:
URL: https://github.com/apache/airflow/pull/15674#discussion_r627218903
##########
File path: airflow/ui/src/components/AppContainer/TimezoneDropdown.tsx
##########
@@ -0,0 +1,90 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Box,
+ Button,
+ Menu,
+ MenuButton,
+ MenuList,
+ Tooltip,
+} from '@chakra-ui/react';
+import dayjs from 'dayjs';
+import tz from 'dayjs/plugin/timezone';
+import Select from 'components/MultiSelect';
+
+import timezones from 'utils/timezones.json';
+import { useTimezoneContext } from 'providers/TimezoneProvider';
+
+dayjs.extend(tz);
+
+interface Option { value: string, label: string }
+
+const TimezoneDropdown: React.FC = () => {
+ const { timezone, setTimezone } = useTimezoneContext();
+ const [now, setNow] = useState(dayjs().tz(timezone));
+
+ let currentTimezone;
+ timezones.find((group) => (
+ group.options.find((zone) => {
+ if (zone.value === timezone) {
+ currentTimezone = zone;
+ return true;
+ }
+ return false;
+ })
+ ));
+
+ const onChangeTimezone = (newTimezone: Option | null) => {
+ if (newTimezone) {
+ setTimezone(newTimezone.value);
+ setNow(dayjs().tz(newTimezone.value));
+ }
+ };
+
+ return (
+ <Menu isLazy>
+ <Tooltip label="Change time zone" hasArrow>
+ <MenuButton as={Button} variant="ghost" mr="4">
+ <Box
+ as="time"
+ dateTime={now.toString()}
+ fontSize="md"
+ >
+ {now.format('h:mmA Z')}
Review comment:
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org