geido commented on code in PR #31310:
URL: https://github.com/apache/superset/pull/31310#discussion_r1891687065
##########
superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx:
##########
@@ -113,17 +106,17 @@ test('can select a timezone values and returns canonical
timezone name', async (
timezone="Africa/Abidjan"
/>,
);
- await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
+ // await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
Review Comment:
One more
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts:
##########
@@ -27,9 +28,11 @@ import {
} from '@superset-ui/core';
import { getComparisonFontSize, getHeaderFontSize } from './utils';
+dayjs.extend(utc);
+
export const parseMetricValue = (metricValue: number | string | null) => {
if (typeof metricValue === 'string') {
- const dateObject = moment.utc(metricValue, moment.ISO_8601, true);
+ const dateObject = dayjs.utc(metricValue, undefined, true);
Review Comment:
Just so I can understand the scope of the change here, what is `undefined`
doing here?
##########
superset-frontend/src/features/alerts/AlertReportModal.test.tsx:
##########
@@ -526,7 +523,9 @@ test('renders default Schedule fields', async () => {
useRedux: true,
});
userEvent.click(screen.getByTestId('schedule-panel'));
- await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
+ if (screen.queryByLabelText('Loading')) {
Review Comment:
I think conditionals in tests might not be the preferred approach. What's
the scenario?
##########
superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx:
##########
@@ -46,20 +41,20 @@ const openSelectMenu = () => {
userEvent.click(searchInput);
};
-jest.spyOn(moment.tz, 'guess').mockReturnValue('America/New_York');
+jest.spyOn(extendedDayjs.tz, 'guess').mockReturnValue('America/New_York');
afterEach(() => {
jest.useRealTimers();
});
-test('use the timezone from `moment` if no timezone provided', async () => {
+test('use the timezone from `dayjs` if no timezone provided', async () => {
const TimezoneSelector = await loadComponent('2022-01-01');
const onTimezoneChange = jest.fn();
render(<TimezoneSelector onTimezoneChange={onTimezoneChange} />);
- expect(screen.getByLabelText('Loading')).toBeVisible();
- await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
+ // expect(screen.getByLabelText('Loading')).toBeVisible();
+ // await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
Review Comment:
Do we need to remove these? If so, what's causing this to be inapplicable?
##########
superset-frontend/src/components/TimezoneSelector/index.tsx:
##########
@@ -82,43 +70,40 @@ export default function TimezoneSelector({
const getTimezoneName = (name: string) => {
const offsets = getOffsetKey(name);
return (
- (currentDate.tz(name).isDST()
+ (isDST(currentDate.tz(name), name)
? offsetsToName[offsets]?.[1]
: offsetsToName[offsets]?.[0]) || name
);
};
- const ALL_ZONES = momentLib.tz
- .countries()
- .map(country => momentLib.tz.zonesForCountry(country, true))
- .flat();
+ // TODO: remove this ts-ignore when typescript is upgraded to 5.1
Review Comment:
What's the TLDR?
##########
superset-frontend/src/pages/ExecutionLogList/index.tsx:
##########
@@ -119,7 +119,7 @@ function ExecutionLog({
original: { scheduled_dttm: scheduledDttm },
},
}: any) =>
Review Comment:
Can we now use a better type than `any`?
##########
superset-frontend/src/pages/AnnotationList/index.tsx:
##########
@@ -170,7 +170,7 @@ function AnnotationList({
row: {
original: { start_dttm: startDttm },
},
- }: any) => moment(new Date(startDttm)).format('ll'),
+ }: any) => dayjs(new Date(startDttm)).format('ll'),
Review Comment:
A good time to remove this `any` for a better type?
##########
superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx:
##########
@@ -71,9 +66,9 @@ test('update to closest deduped timezone when timezone is
provided', async () =>
timezone="America/Los_Angeles"
/>,
);
- await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
+ // await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
Review Comment:
Same as my other comment
##########
superset-frontend/src/pages/ExecutionLogList/index.tsx:
##########
@@ -128,7 +128,7 @@ function ExecutionLog({
row: {
original: { start_dttm: startDttm },
},
- }: any) => moment(new Date(startDttm)).format('YYYY-MM-DD hh:mm:ss a'),
+ }: any) => dayjs(new Date(startDttm)).format('YYYY-MM-DD hh:mm:ss a'),
Review Comment:
One more about `any`
##########
superset-frontend/src/pages/AnnotationList/index.tsx:
##########
@@ -179,7 +179,7 @@ function AnnotationList({
row: {
original: { end_dttm: endDttm },
},
- }: any) => moment(new Date(endDttm)).format('ll'),
+ }: any) => dayjs(new Date(endDttm)).format('ll'),
Review Comment:
Same comment about removing `any`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
