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

Bump antd to 3.26.20 #1247

Merged
merged 3 commits into from
Mar 10, 2023
Merged

Conversation

mszabo-wikia
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

With the TS update it now seems possible to update antd. There are two eventual benefits:

  • antd v4 contains React 18-compatible typings,
  • antd v4 supports treeshakeable SVG icons, reducing the bundle size.

The migration guide recommends updating to the latest v3 release before going to v4.

Apart from ubiquitous snapshot changes, a few adjustments were necessary in components and tests:

  • The test case for the <Page /> component that aimed to verify that the header was not rendered in embedded mode was only working by accident since 53937eb—the Header component didn't have a display name, so the test wouldn't find it even though the test setup was no longer correct. Fix the test setup to setup the embedded prop accordingly.
  • antd's Menu component is now wrapped in a Context.Consumer, so tests using enzyme's shallow rendering need to dive into it to be able to find the child components they are looking for.
  • The Upload component has more strictly typed props now, so the signature of the loadJsonFile prop had to be adjusted accordingly. The handling code was only accessing the file property of FileList, and the real type supplied by the event handler also has a file property, so no code changes were required apart from the typing update.
  • The Table component has one more internal wrapper (each passing down the className prop to the next), so the test in TraceSpanView had to be updated accordingly.
  • The onCell event handler for Tables is now strictly typed, so DetailTable needed an update to return an empty object {} if it intends to set no props. This is consistent with the upstream examples.
  • KeyboardShortcutsHelp was setting a spurious align prop on Modal, which was already not part of its typings in 3.9.0 but was still present as a prop type. This caused a type error now that the prop type was removed.
  • In manual testing, I observed that the name selectors (e.g. "Group By") on the trace statistics view were not functioning—they would immediately close when clicking to open them. I thought this was a regression caused by the update, but experienced the same behavior on main. This was fixed by deferring event handler registration to ensure the event handlers don't end up catching the same event that triggered the original state change.

Screenshots

Screenshot 2023-03-09 at 23 05 09
Screenshot 2023-03-09 at 23 05 28
Screenshot 2023-03-09 at 23 05 45
Screenshot 2023-03-09 at 23 06 07

Signed-off-by: Máté Szabó <mszabo@fandom.com>
Signed-off-by: Máté Szabó <mszabo@fandom.com>
Signed-off-by: Máté Szabó <mszabo@fandom.com>
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Patch coverage: 42.85% and project coverage change: -0.26 ⚠️

Comparison is base (ee7ce64) 95.61% compared to head (a9c01f8) 95.35%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1247      +/-   ##
==========================================
- Coverage   95.61%   95.35%   -0.26%     
==========================================
  Files         244      244              
  Lines        7749     7750       +1     
  Branches     2017     2017              
==========================================
- Hits         7409     7390      -19     
- Misses        340      360      +20     
Impacted Files Coverage Δ
...r-ui/src/components/SearchTracePage/FileLoader.tsx 100.00% <ø> (ø)
...racePage/TracePageHeader/KeyboardShortcutsHelp.tsx 100.00% <ø> (ø)
...s/jaeger-ui/src/components/common/NameSelector.tsx 90.24% <20.00%> (-9.76%) ⬇️
.../src/components/common/DetailsCard/DetailTable.tsx 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

rockstar!

could you point me to how snapshots are regenerated? we should have it documented

@yurishkuro yurishkuro merged commit cda1746 into jaegertracing:main Mar 10, 2023
@mszabo-wikia
Copy link
Contributor Author

Good idea, filed #1251 for that.

@mszabo-wikia mszabo-wikia deleted the update-antd branch March 10, 2023 21:29
Binrix pushed a commit to Binrix/jaeger-ui that referenced this pull request Apr 18, 2023
## Which problem is this PR solving?
- Contributes towards jaegertracing#1199 

## Short description of the changes
With the TS update it now seems possible to update `antd`. There are two
eventual benefits:
* antd v4 contains React 18-compatible typings,
* antd v4 supports [treeshakeable SVG
icons](https://github.com/ant-design/ant-design/blob/4.24.8/docs/react/migration-v4.en-US.md#icon-upgrade),
reducing the bundle size.

The [migration
guide](https://github.com/ant-design/ant-design/blob/4.24.8/docs/react/migration-v4.en-US.md)
recommends updating to the latest v3 release before going to v4.

Apart from ubiquitous snapshot changes, a few adjustments were necessary
in components and tests:
* The test case for the `<Page />` component that aimed to verify that
the header was not rendered in embedded mode was only working by
accident since 53937eb—the `Header`
component didn't have a display name, so the test wouldn't find it even
though the test setup was no longer correct. Fix the test setup to setup
the `embedded` prop accordingly.
* antd's `Menu` component is now wrapped in a `Context.Consumer`, so
tests using enzyme's shallow rendering need to `dive` into it to be able
to find the child components they are looking for.
* The `Upload` component has more strictly typed props now, so the
signature of the `loadJsonFile` prop had to be adjusted accordingly. The
handling code was only accessing the `file` property of `FileList`, and
the real type supplied by the event handler also has a `file` property,
so no code changes were required apart from the typing update.
* The `Table` component has one more internal wrapper (each passing down
the `className` prop to the next), so the test in `TraceSpanView` had to
be updated accordingly.
* The `onCell` event handler for `Table`s is now strictly typed, so
`DetailTable` needed an update to return an empty object `{}` if it
intends to set no props. This is consistent with the upstream
[examples](https://github.com/react-component/table/blob/83cce50ddb0a55d63bb0f435d3f4ff7e3301ebad/docs/examples/colspan-rowspan.tsx#L25).
* `KeyboardShortcutsHelp` was setting a spurious `align` prop on
`Modal`, which was already not part of its typings in 3.9.0 but was
still present as a prop type. This caused a type error now that the prop
type was removed.
* In manual testing, I observed that the name selectors (e.g. "Group
By") on the trace statistics view were not functioning—they would
immediately close when clicking to open them. I thought this was a
regression caused by the update, but experienced the same behavior on
`main`. This was fixed by deferring event handler registration to ensure
the event handlers don't end up catching the same event that triggered
the original state change.

## Screenshots
![Screenshot 2023-03-09 at 23 05
09](https://user-images.githubusercontent.com/2721291/224171058-cda04376-b4b0-4fbd-84db-e190c4980394.png)
![Screenshot 2023-03-09 at 23 05
28](https://user-images.githubusercontent.com/2721291/224171065-55553175-c47c-4d6b-995b-2911715be1bb.png)
![Screenshot 2023-03-09 at 23 05
45](https://user-images.githubusercontent.com/2721291/224171068-ed533a61-ccb9-4f1c-8a0c-8e0088826b79.png)
![Screenshot 2023-03-09 at 23 06
07](https://user-images.githubusercontent.com/2721291/224171072-ec84393d-e402-4bd2-b422-cd37b8e21977.png)

---------

Signed-off-by: Máté Szabó <mszabo@fandom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants