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

Upgrade ant-design to v5.x #1907

Merged
merged 8 commits into from
Oct 24, 2023
Merged

Conversation

anshgoyalevil
Copy link
Contributor

@anshgoyalevil anshgoyalevil commented Oct 24, 2023

Which problem is this PR solving?

Description of the changes

  • The Size of this PR may look big, but there were no breaking changes except the following:
    • ant-design v5.0.0 no longer supports less variables. That means we have to use the seedToken and aliasToken in place of less variables.
    • With this change, we also needed to use the default styling of ant-design v4. For that, ant-design team provides a compatible package.
    • The notification.close and warn have been renamed destory and warning respectively.
  • PR: https://github.com/jaegertracing/jaeger-ui/pull/1839/files is reversed in this PR, because we are not really on @types/react v18. We are locked on v16 due to yarn resolution. Having v18 written in package.json was creating conflict with the installation of antd v5
screen-capture.5.webm

How was this change tested?

  • Manually, and unit tests

Checklist

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
@anshgoyalevil anshgoyalevil marked this pull request as ready for review October 24, 2023 05:57
@anshgoyalevil anshgoyalevil requested a review from a team as a code owner October 24, 2023 05:57
@anshgoyalevil anshgoyalevil requested review from joe-elliott and removed request for a team October 24, 2023 05:57
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
packages/jaeger-ui/src/components/App/TopNav.tsx 94.59% <ø> (ø)
packages/jaeger-ui/src/components/App/index.jsx 100.00% <100.00%> (ø)
...s/DeepDependencies/Header/LayoutSettings/index.tsx 100.00% <ø> (ø)
...src/components/TracePage/ArchiveNotifier/index.tsx 100.00% <100.00%> (ø)
...TracePage/TraceTimelineViewer/ReferencesButton.tsx 100.00% <ø> (ø)
...kages/jaeger-ui/src/components/common/CopyIcon.tsx 100.00% <ø> (ø)
...eger-ui/src/components/common/SearchableSelect.tsx 100.00% <ø> (ø)

📢 Thoughts on this report? Let us know!.

@anshgoyalevil
Copy link
Contributor Author

@yurishkuro The size of this PR is quite big, but I wasn't able to distill it down to multiple PRs, because all the breaking changes were introduced in v5.0.0. v5.latest didn't bring any other breaking changes, except the styling changes, which we are already on v4.

The size grew this big due to snapshot changes 📷

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
@@ -146,6 +146,7 @@ export function TopNavImpl(props: Props) {
disabledOverflow
selectedKeys={[pathname]}
items={itemsGlobalRight}
style={{ color: '#fff' }}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be in css somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but, since the change was a one-liner, I thought of adding an inline style. Moving it to the CSS file.

@@ -64,33 +64,6 @@ export default defineConfig({
less: {
math: 'always',
javascriptEnabled: true,
modifyVars: {
// Supply appropriate overrides to the Ant Design System.
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what's happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we are basically removing the replacement variables.

Before v5, we had an option to replace less variables in ant-design's core files with the ones defined here.
This modifyVars object was containing all such variables. But since v5, antd doesn't support less variables, so, this modification thing is of no use.

Instead, ant-design introduced seedTokens and aliasTokens which can be supplied to the theme directly.

Copy link
Member

Choose a reason for hiding this comment

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

is there documentation about that? The names seedToken don't tell me much, especially why they would be a replacement for less variables.

On a high level, what is the approach now? My understanding was that less variables allowed us to reuse settings across different css files. Do we have to replicate those now in many places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ant-design v4 to v5 migration guide explaining why they removed the support for less variables.

This document explains in deep how to migrate from less variables to tokens and what exactly is all that seedToken, and other such token thing.

Since we were replacing those variables via the modifyVars object, the same can be (is achieved) using the design tokens supplied to the theme configuration of ant-design.

less variables for sure achieves the purpose of reusing them across files, but since the change is internal to ant-design library, we aren't affected in other areas, as we aren't using any antd's less file in our code.

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
@yurishkuro yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Oct 24, 2023
@yurishkuro yurishkuro merged commit 2a91537 into jaegertracing:main Oct 24, 2023
10 of 11 checks passed
@yurishkuro
Copy link
Member

🚀 🚀 🚀

@anshgoyalevil anshgoyalevil deleted the 7antd branch October 25, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Antd
2 participants