refactor(date picker): Migrate Date Picker to Ant Design 5 #31019

msyavuz · 2024-11-21T14:24:27Z

SUMMARY

Migrate Datepicker to Ant Design 5 while also finishing the migration to Dayjs in Datepicker related components.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

Run the testing suite / Check for visual changes

ADDITIONAL INFORMATION

Has associated issue: Fixes Ant Design 5: Upgrade DatePicker component #30910
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

…ry loading

…com/msyavuz/superset into msyavuz/refactor/migrate-datepicker

superset-frontend/webpack.config.js

kgabryje · 2025-01-07T13:03:08Z

Can't believe we're finally getting rid of moment.js! Amazing job @msyavuz. The code looks good to me besides a small nitpick, manual testing in progress

superset-frontend/packages/superset-ui-chart-controls/src/constants.ts

kgabryje · 2025-01-07T13:45:54Z

Docker build keeps failing, but I can compile the code locally without any problem. @mistercrunch can it be related to your recent docker changes?

kgabryje · 2025-01-09T13:17:48Z

/testenv up

github-actions · 2025-01-09T13:18:01Z

@kgabryje Processing your ephemeral environment request here.

github-actions · 2025-01-09T13:26:30Z

@kgabryje Ephemeral environment spinning up at http://34.209.140.91:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

kgabryje · 2025-01-09T16:21:25Z

@msyavuz I'm getting an error when I do the following:

add a time range filter on a dashboard
open that filter
go to "custom"

Interestingly, I can just close this error (with x in top right corner) and then everything seems to be working fine - I'm able to select a date with the date picker

msyavuz · 2025-01-09T19:46:48Z

@msyavuz I'm getting an error when I do the following:
* add a time range filter on a dashboard

* open that filter

* go to "custom"
Interestingly, I can just close this error (with x in top right corner) and then everything seems to be working fine - I'm able to select a date with the date picker

@kgabryje This revealed a deeper issue which is not using locales in all datepickers. I extracted a useLocale hook from CustomFrame to enable setting locale individually on other places. So now filter control datepickers should respect the locale as well. Also fixed the error :)

kgabryje · 2025-01-10T05:16:21Z

superset-frontend/webpack.config.js

@@ -160,11 +160,6 @@ const plugins = [
    chunks: [],
    filename: '500.html',
  }),
-  // Dayjs locales to keep
-  new webpack.ContextReplacementPlugin(


Shouldn’t we keep this plugin to trim unsupported locales from the bundle?

kgabryje

Looks great! Thanks again for the contribution!

superset-frontend/src/components/TimezoneSelector/index.tsx

mistercrunch · 2025-01-14T22:50:05Z

superset-frontend/src/components/ListView/Filters/DateRange.tsx

-        }}
-      />
-    </RangeFilterContainer>
+    <AntdThemeProvider locale={locale}>


Noticed this while rebasing my large theming PR, but we won't want to re-inject the AntdThemeProvider in multiple places. My PR will clean this up and load it once and only once

I did that because I don't know if modifying global theme provider is something we want. Some components might not want to be localized.

Yes, looking at what is needed here it seems it will fetch some local configuration async and isn't ready to render until those locale files are loaded, so we really don't want that at the base of the tree (aka in the main ThemeProvider) and delay the first render. It's important to not hold up the main tree which is what is happening here.

Now, knowing that it must be inject more dynamically or on-demand, it'd be great for it to be DRY (single injection point if/where needed) and for the fetch-locale logic to be cached to it only happens once in the SPA. I didn't review the logic here to see whether there's any caching, but squeezing it in the client-side cache somewhere seems important.

Now how should the components that need it get it? Maybe some sort of reusable AntdLocaleProvider decorator or something simple like that, where components that need would just wrap themselves with it (?) That wrapper itself could take care of the caching with some way to share that locale object with other instances of that same component.

mistercrunch · 2025-01-14T22:50:35Z

superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx

-                    ? (triggerNode.parentNode as HTMLElement)
-                    : document.body
-                }
+    <AntdThemeProvider locale={datePickerLocale}>


same here, we don't want to sprinkly theme providers in the react tree...

mistercrunch · 2025-01-14T22:52:13Z

superset-frontend/src/hooks/useLocale.ts

+import { useEffect, useState } from 'react';
+import { useSelector } from 'react-redux';
+import { ExplorePageState } from 'src/explore/types';
+import 'dayjs/locale/en';


how bad is all this on bundle size?

Bundle size is not affected since we used to import these before as well

@mistercrunch I verified this before approving, the PR decreases the bundle size thanks to removing moment, the size of those locales is negligible

refactor(date picker): initial commit

2b1950a

pull-request-size bot added the size/L label Nov 21, 2024

github-actions bot added the dependencies:npm label Nov 21, 2024

sadpandajoe added the review:draft label Nov 21, 2024

geido mentioned this pull request Nov 27, 2024

Remove Moment.js in favor of Day.js #31178

Closed

geido added the preset:bounty Issues that have been selected by Preset and have a bounty attached. label Dec 2, 2024

msyavuz added 8 commits December 5, 2024 21:20

refactor(moment): Replace Moment.js with DayJs

ba7a325

fix(package): sync package.json and tests

ded1285

refactor(moment): refactor moment out of components

63d7c02

refactor(dayjs): re-add moment and migrate components

17d0041

refactor(dayjs): migrate some components

8eb99d3

chore(moment): add comment and change moment to dayjs

f556d9c

refactor(dayjs): centralize dayjs extensions

8572277

chore(dayjs): add test for extensions and change package.jsons

fd2f451

msyavuz mentioned this pull request Dec 17, 2024

refactor(moment): Replace Moment.js with DayJs #31310

Merged

9 tasks

msyavuz added 15 commits December 18, 2024 00:26

fix(type): type from dayjs instead

4a55d06

Merge branch 'master' into msyavuz/momentjs-to-dayjs

9c8eca1

fix(test): only wait for load if its loading

6cbac28

test(AlertReportModal): use events directly instead?

e9fcacd

test(AlertReportModal): fix the test

a035c87

refactor(pluigin-echarts): move to dayjs

af97ee0

chore: remove unused waitfor

5a0862a

feat(moment): remove timezone-support, migrate handlebars to dayjs

a3e10bf

chore(timezone-support): remove usages

638599f

refactor(legacy-plugins): migrate to dayjs

854a66a

fix(timezoneselector): use map instead of array and remove unneccessa…

972bf08

…ry loading

refactor(alert report modal): remove loading check

58a7355

chore(alert report modal): remove unused import

f60a221

Merge branch 'msyavuz/refactor/migrate-datepicker' of https://github.…

568b4f5

…com/msyavuz/superset into msyavuz/refactor/migrate-datepicker

chore(package): remove package manager

962ad20

kgabryje reviewed Jan 7, 2025

View reviewed changes

superset-frontend/webpack.config.js Outdated Show resolved Hide resolved

kgabryje reviewed Jan 7, 2025

View reviewed changes

superset-frontend/packages/superset-ui-chart-controls/src/constants.ts Outdated Show resolved Hide resolved

msyavuz added 6 commits January 8, 2025 15:46

chore(timezones): remove deduping logic

79925f3

chore(moment): remove moment locales plugin

b3d0a24

Merge branch 'master' into msyavuz/refactor/migrate-datepicker

4939091

refactor(invalid date): turn to sentence case

3de0a5d

test(TimezoneSelector): change test for all timezones

5835167

fix(TimezoneSelector): change tests for all timezones

ace0177

feat(useLocale): add useLocale hook to unify locale

5b22577

msyavuz added 2 commits January 9, 2025 23:08

test(ListView): provide store

f5a699d

chore(webpack): remove unused function

32f2d46

kgabryje reviewed Jan 10, 2025

View reviewed changes

msyavuz added 2 commits January 10, 2025 09:48

feat(webpack): add ignore plugin

246ad4d

fix(webpack): import dayjs locales directly instead of webpack plugin

407a819

kgabryje approved these changes Jan 13, 2025

View reviewed changes

geido reviewed Jan 13, 2025

View reviewed changes

superset-frontend/src/components/TimezoneSelector/index.tsx Outdated Show resolved Hide resolved

geido and others added 2 commits January 13, 2025 13:37

Update superset-frontend/src/components/TimezoneSelector/index.tsx

c6b6aab

chore(format): remove line

ddb7af8

geido merged commit b5e6275 into apache:master Jan 13, 2025
50 checks passed

mistercrunch reviewed Jan 14, 2025

View reviewed changes

msyavuz mentioned this pull request Jan 15, 2025

fix: refactor(date picker): Migrate Date Picker to Ant Design 5 #31839

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(date picker): Migrate Date Picker to Ant Design 5 #31019

refactor(date picker): Migrate Date Picker to Ant Design 5 #31019

msyavuz commented Nov 21, 2024 •

edited

Loading

kgabryje commented Jan 7, 2025

kgabryje commented Jan 7, 2025

kgabryje commented Jan 9, 2025

github-actions bot commented Jan 9, 2025

github-actions bot commented Jan 9, 2025

kgabryje commented Jan 9, 2025 •

edited

Loading

msyavuz commented Jan 9, 2025 •

edited

Loading

kgabryje Jan 10, 2025

kgabryje left a comment

mistercrunch Jan 14, 2025

msyavuz Jan 15, 2025

mistercrunch Jan 15, 2025 •

edited

Loading

mistercrunch Jan 14, 2025

mistercrunch Jan 14, 2025

msyavuz Jan 15, 2025

kgabryje Jan 15, 2025 •

edited

Loading

refactor(date picker): Migrate Date Picker to Ant Design 5 #31019

refactor(date picker): Migrate Date Picker to Ant Design 5 #31019

Conversation

msyavuz commented Nov 21, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

kgabryje commented Jan 7, 2025

kgabryje commented Jan 7, 2025

kgabryje commented Jan 9, 2025

github-actions bot commented Jan 9, 2025

github-actions bot commented Jan 9, 2025

kgabryje commented Jan 9, 2025 • edited Loading

msyavuz commented Jan 9, 2025 • edited Loading

kgabryje Jan 10, 2025

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

mistercrunch Jan 14, 2025

Choose a reason for hiding this comment

msyavuz Jan 15, 2025

Choose a reason for hiding this comment

mistercrunch Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

mistercrunch Jan 14, 2025

Choose a reason for hiding this comment

mistercrunch Jan 14, 2025

Choose a reason for hiding this comment

msyavuz Jan 15, 2025

Choose a reason for hiding this comment

kgabryje Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

msyavuz commented Nov 21, 2024 •

edited

Loading

kgabryje commented Jan 9, 2025 •

edited

Loading

msyavuz commented Jan 9, 2025 •

edited

Loading

mistercrunch Jan 15, 2025 •

edited

Loading

kgabryje Jan 15, 2025 •

edited

Loading