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 to antd/v3.9.0 to avoid loading fonts from alicdn #1053

Merged
merged 5 commits into from
Nov 14, 2022

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Nov 12, 2022

Resolves #187

Despite the concerns in the original ticket, it does not seem to affect the bundle size, because SVG icons are inlined, so the full font file is not added to the bundle.

$ du -h cmd/query/app/ui/actual/
3.8M	cmd/query/app/ui/actual/

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@codecov
Copy link

codecov bot commented Nov 13, 2022

Codecov Report

Base: 95.37% // Head: 95.30% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (ef6708b) compared to base (c863508).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1053      +/-   ##
==========================================
- Coverage   95.37%   95.30%   -0.07%     
==========================================
  Files         243      243              
  Lines        7561     7561              
  Branches     1895     1895              
==========================================
- Hits         7211     7206       -5     
- Misses        343      348       +5     
  Partials        7        7              
Impacted Files Coverage Δ
...aeger-ui/src/components/App/TraceIDSearchInput.tsx 100.00% <ø> (ø)
...es/jaeger-ui/src/components/common/UiFindInput.tsx 100.00% <ø> (ø)
...s/jaeger-ui/src/components/common/NameSelector.tsx 100.00% <100.00%> (ø)
...mponents/TracePage/TraceStatistics/tableValues.tsx 93.79% <0.00%> (-1.73%) ⬇️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yurishkuro
Copy link
Member Author

@albertteoh the most changes are in packages/jaeger-ui/src/components/Monitor/ServicesView/operationDetailsTable/snapshots/index.test.js.snap. How much are the icons used in there? I'm surprised antd inlines the SVGs instead of defining them in the style. If this happens many times per row, it might result in a lot of bulk. But if it's only in the headers of the table then should be ok.

@albertteoh
Copy link
Contributor

How much are the icons used in there?

Yes, those icons only exist in the table's header.

Based on the diff, the changes relate to just the table header (the "info" icon and the up/down caret icons for sorting the rows). My understanding of why there are a lot of changes is that there's ~10 snapshots (from what I see in the diff), with each snapshot having 2*5 up/down caret updates + 1 "info" icon; then each icon change taking up ~20 lines of changes.

So a back of the envelope calc is about 10 * 11 * 20 ~= 2200 which is around the vicinity of the line count in the diff on that file (2471).

@yurishkuro yurishkuro merged commit f3f324d into jaegertracing:main Nov 14, 2022
@yurishkuro yurishkuro deleted the antd39 branch November 14, 2022 02:00
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.

Remove the dependency on alicdn.com
2 participants