Upgrade antd to v4.15.6 #1748

anshgoyalevil · 2023-09-01T10:44:23Z

Which problem is this PR solving?

part of: Upgrade antd to 4.24.13 #1734
This PR is a part of upgrading antd to latest version of 4.x

Description of the changes

Disabled the NavBar hover effect which was un-necessarily introduced
NavBar's height was increased internally. Fixed it to match previous height (48px)
The Monitor -> EmptyPage was using the offsets to align content to the center, but it is not required now, as it is a default behavior
SorterResult API is removed, so used lodash in place of that
CompareFn typedef is no longer required
Mocked the window.matchMedia, since some tests were using it
dataIndex now requires an array instead of nested string. Ex: "abc.def.ghi" -> ["abc", "def", "ghi"]
Tests are updated according to these changes
Snapshots are updated according to internal changes in components

How was this change tested?

Manually, and using automated testing

What's next?

In the next PRs, the antd would be upgraded to 4.20.0 (or higher), to minimise the changes in a single PR.
New type errors would be introduced thereby, like NumberInput lookback changes, etc.

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

codecov · 2023-09-01T10:56:55Z

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed	Coverage
...ger-ui/src/components/Monitor/EmptyState/index.tsx	`ø`
...s/TracePage/TracePageHeader/TracePageSearchBar.tsx	`ø`
...nitor/ServicesView/operationDetailsTable/index.tsx	`100.00%`
...r-ui/src/components/SearchTracePage/FileLoader.tsx	`100.00%`
...mponents/TraceDiff/TraceDiffHeader/CohortTable.tsx	`100.00%`
...i/src/components/TracePage/TraceSpanView/index.tsx	`100.00%`
...src/components/TracePage/TraceStatistics/index.tsx	`100.00%`
.../src/components/common/DetailsCard/DetailTable.tsx	`100.00%`

📢 Thoughts on this report? Let us know!.

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro · 2023-09-01T17:09:01Z

packages/jaeger-ui/src/components/Monitor/EmptyState/index.tsx

@@ -32,7 +32,7 @@ export default class MonitorATMEmptyState extends React.PureComponent {
    return (
      <Col>
        <Row justify="center">
-          <Col span={8} offset={8}>


doesn't removing offset change the position of the column? When I see <Col span={8} offset={8}> I interpret it "start from column 8 and span columns 8-15"

Previously (in antd 3x), the <Col span={8}> was interpreted as, "The width of column should be 8 boxes out of the 24 boxes in a grid".
That offset property defines that those 8 boxes must start from the 9th box out of those 24 boxes so that basically centers the col.

In antd 4x, this behavior is controlled by <Row justify="center">. Don't know why it was not working previously in 3x, even though it existed before too.

Here's what the page looks like now:

yurishkuro · 2023-09-01T17:14:49Z

packages/jaeger-ui/src/components/Monitor/ServicesView/operationDetailsTable/index.tsx

+
+            if (!isEqual(activeSorters, tableSorting)) {
+              const lastColumn =
+                activeSorters[activeSorters.length - 1] ?? tableSorting[tableSorting.length - 1];


if both arrays are empty, isn't it possible to end up with lastColumn = undefined?

But, I think, in that case, the onChange function wouldn't be called.
Whenever someone clicks the table header for sorting, the tableSorting or activeSorters would always return an array of size greater than or equal to 1.

yurishkuro · 2023-09-01T17:15:33Z

packages/jaeger-ui/src/components/Monitor/ServicesView/operationDetailsTable/index.tsx

-          onChange={(pagination, filters, { columnKey, order }) => {
-            if (!isEqual({ columnKey, order }, this.state.tableSorting)) {
-              const clickedColumn = tableTitles.get(columnKey || this.state.tableSorting.columnKey);
+          onChange={(pagination, filters, sorter) => {


what specifically is changing in antd that requires these modifications, which look more complicated than before?

In antd 4x, sorting by multiple sortable columns now reports all columns that are active in sorting, so the event handling code must also accept an array.

packages/jaeger-ui/src/components/SearchTracePage/FileLoader.tsx

yurishkuro · 2023-09-01T17:21:18Z

packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageSearchBar.test.js

    });

    it('hides navigation buttons when not navigable', () => {
      wrapper.setProps({ navigable: false });
      const button = wrapper.find('Button');
      expect(button.length).toBe(1);
-      expect(button.prop('icon')).toBe('close');
+      expect(button.getElement(CloseOutlined)).toBeDefined();


can you not look it up by data-testid to avoid explicit dependency on ant icons?

Yes, maybe it could be done that way too. I will try and update.

packages/jaeger-ui/vite.config.ts

yurishkuro · 2023-09-01T17:26:22Z

packages/jaeger-ui/src/components/TracePage/TraceSpanView/index.tsx

@@ -133,7 +133,7 @@ export default class TraceSpanView extends Component<Props, State> {
    const columns: ColumnProps<Span>[] = [
      {
        title: 'Service Name',
-        dataIndex: 'process.serviceName',
+        dataIndex: ['process', 'serviceName'],


is this our own notation or something that antd component supports, like a nested indexing path into a data structure?

antd dataIndex definition changes from 'xxx.yyy' to ['xxx', 'yyy'].
So basically, antd now want us to supply dataIndex path in the form of an array for nested indexing pathnames.

Here's a link to that: https://4x.ant.design/docs/react/migration-v4#:~:text=Nest%20dataIndex%20definition%20changes%20from%20%27xxx.yyy%27%20to%20%5B%27xxx%27%2C%20%27yyy%27%5D

yurishkuro · 2023-09-01T17:27:24Z

packages/jaeger-ui/src/components/TracePage/TraceSpanView/index.tsx

-                  return <Option key={name}>{name} </Option>;
+                  return (
+                    <Option value={name} key={name}>
+                      {name}{' '}


just curious, what's the need for the extra space in the display string?

Previously it was return <Option key={name}>{name} </Option>; (Notice an extra space after {name}).
So, on running yarn fmt, prettier detected that maybe that space is needed, and formatted it that way ({' '})

yurishkuro · 2023-09-01T17:28:11Z

packages/jaeger-ui/src/components/TracePage/TraceSpanView/index.test.js

@@ -38,7 +38,7 @@ describe('<TraceSpanView>', () => {
  it('does not explode', () => {
    expect(wrapper).toBeDefined();
    expect(wrapper.find('.title--TraceSpanView').length).toBe(1);
-    expect(wrapper.find('.span-view-table').length).toBe(3);
+    expect(wrapper.find('.span-view-table').length).toBe(2);


why is this count changing?

Since the re-write of the Table in 4.x, there doesn't exist an extra withStore(Table) wrapper with the class name span-view-table.

anshgoyalevil · 2023-09-02T08:43:39Z

packages/jaeger-ui/src/components/App/Page.css

+
+.ant-menu-item:hover {
+  background-color: transparent !important;
+}


In antd 4x, the TopNav contains the hover effect by default, which is not what we had earlier.
This CSS property reverts that behavior.

don't understand this one. The About Jaeger menu also self-activates on hover.

The About Jaeger menu also self-activates on hover.

I just checked the live build on the previous version (3.x) without any change. It showed the same behavior there too.
If that was a bug, I would fix it in next PR.

About that CSS change, here's how it behaves without it:

screen-capture.1.webm

With it:

screen-capture.2.webm

anshgoyalevil · 2023-09-02T08:55:20Z

packages/jaeger-ui/src/components/Monitor/ServicesView/index.css

-  position: absolute;
-  bottom: 14px;


The position of the Timeframe Selector needs to be relative to the Col beside it.

Due to position: absolute, the Selector was going to the bottom left corner, which is not the desired behavior.

bottom: 14px is removed because it is already at an appropriate distance from the col below it.

In Antd 3x:

In antd 4x, without changes:

With this change:

anshgoyalevil · 2023-09-02T08:58:30Z

packages/jaeger-ui/src/components/TracePage/TracePageHeader/KeyboardShortcutsHelp.css

-  margin: 0 -7px;
+  margin: -3px -7px;


This is to center the Keyboard shortcut panel icon vertically.

Without this:

With this:

anshgoyalevil · 2023-09-02T09:02:37Z

packages/jaeger-ui/vite.config.ts

-          '@layout-header-height': '64px',
+          '@layout-header-height': '48px',


The Header's height was increased with 4.x upgrade. The resulting NavBar wasn't the one we had earlier. To revert this internal change, the height has to be decreased here.

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro · 2023-09-02T15:44:19Z

Code coverage drop is due to this:

Is this a flaky test or the result of the upgrade?

yurishkuro · 2023-09-02T15:46:28Z

packages/jaeger-ui/src/components/Monitor/EmptyState/index.tsx

@@ -59,7 +59,7 @@ export default class MonitorATMEmptyState extends React.PureComponent {
            )}
            {this.config.alert && (
              <Row justify="center">


is this correct that we have a row inside a row?

As far as I am seeing, it's giving the expected result. Not sure 100% though, as there is not a strict guide about nested row and cols.

anshgoyalevil · 2023-09-02T15:51:17Z

Code coverage drop is due to this: Is this a flaky test or the result of the upgrade?

Don't know why but its giving 100 % coverage locally.

The issue's is not with the test, but the codecov. I have seen this behaviour in many places.

yurishkuro · 2023-09-02T15:52:24Z

🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳

yurishkuro · 2023-09-02T15:59:26Z

I rebased #1636, which bumps antd even further - may fail on other issues

anshgoyalevil · 2023-09-02T16:03:43Z

I rebased #1636, which bumps antd even further - may fail on other issues

Yes, antd requires another manual bump. The dependabot's PR would fail for now.

yurishkuro · 2023-09-04T06:08:29Z

part of #1703

upgrade to antd-4.15.6

947ae90

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil requested review from everett980 and rubenvp8510 as code owners September 1, 2023 10:44

anshgoyalevil added 2 commits September 1, 2023 16:30

fix TracePageHeader icons

8559bb5

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

Merge branch 'main' into antd-4x

27d6c7f

yurishkuro reviewed Sep 1, 2023

View reviewed changes

packages/jaeger-ui/src/components/SearchTracePage/FileLoader.tsx Show resolved Hide resolved

yurishkuro reviewed Sep 1, 2023

View reviewed changes

packages/jaeger-ui/vite.config.ts Show resolved Hide resolved

yurishkuro reviewed Sep 1, 2023

View reviewed changes

anshgoyalevil commented Sep 2, 2023

View reviewed changes

add test for beforeUpload and fix Icon related test

cac01ae

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro reviewed Sep 2, 2023

View reviewed changes

yurishkuro approved these changes Sep 2, 2023

View reviewed changes

yurishkuro merged commit 91969a7 into jaegertracing:main Sep 2, 2023
7 of 8 checks passed

anshgoyalevil mentioned this pull request Sep 3, 2023

Upgrade antd to v4.16.0 from v4.15.6 #1750

Merged

4 tasks

anshgoyalevil mentioned this pull request Sep 4, 2023

Upgrade antd to 4.24.13 #1734

Closed

4 tasks

anshgoyalevil deleted the antd-4x branch September 4, 2023 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade antd to v4.15.6 #1748

Upgrade antd to v4.15.6 #1748

anshgoyalevil commented Sep 1, 2023

codecov bot commented Sep 1, 2023 •

edited

yurishkuro Sep 1, 2023

anshgoyalevil Sep 2, 2023

yurishkuro Sep 1, 2023

anshgoyalevil Sep 2, 2023

yurishkuro Sep 1, 2023

anshgoyalevil Sep 2, 2023

yurishkuro Sep 1, 2023

anshgoyalevil Sep 2, 2023

yurishkuro Sep 1, 2023

anshgoyalevil Sep 2, 2023

yurishkuro Sep 1, 2023

anshgoyalevil Sep 2, 2023

yurishkuro Sep 1, 2023

anshgoyalevil Sep 2, 2023 •

edited

anshgoyalevil Sep 2, 2023 •

edited

yurishkuro Sep 2, 2023

anshgoyalevil Sep 2, 2023 •

edited

yurishkuro Sep 2, 2023

anshgoyalevil Sep 2, 2023 •

edited

anshgoyalevil Sep 2, 2023

anshgoyalevil Sep 2, 2023

anshgoyalevil Sep 2, 2023

yurishkuro commented Sep 2, 2023

yurishkuro Sep 2, 2023

anshgoyalevil Sep 2, 2023

anshgoyalevil commented Sep 2, 2023

yurishkuro commented Sep 2, 2023

yurishkuro commented Sep 2, 2023

anshgoyalevil commented Sep 2, 2023

yurishkuro commented Sep 4, 2023

		'@layout-header-height': '64px',
		'@layout-header-height': '48px',

Upgrade antd to v4.15.6 #1748

Upgrade antd to v4.15.6 #1748

Conversation

anshgoyalevil commented Sep 1, 2023

Which problem is this PR solving?

Description of the changes

How was this change tested?

What's next?

Checklist

codecov bot commented Sep 1, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshgoyalevil Sep 2, 2023 • edited

Choose a reason for hiding this comment

anshgoyalevil Sep 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshgoyalevil Sep 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshgoyalevil Sep 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Sep 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshgoyalevil commented Sep 2, 2023

yurishkuro commented Sep 2, 2023

yurishkuro commented Sep 2, 2023

anshgoyalevil commented Sep 2, 2023

yurishkuro commented Sep 4, 2023

codecov bot commented Sep 1, 2023 •

edited

anshgoyalevil Sep 2, 2023 •

edited

anshgoyalevil Sep 2, 2023 •

edited

anshgoyalevil Sep 2, 2023 •

edited

anshgoyalevil Sep 2, 2023 •

edited