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 react-icons from 2.2.7 to 4.10.1 #1721

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

anshgoyalevil
Copy link
Member

Which problem is this PR solving?

Checklist

Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
@anshgoyalevil
Copy link
Member Author

anshgoyalevil commented Aug 22, 2023

@yurishkuro I have included the icon changes.

The previous icons were not available to look up somewhere (yes, 2nd option was to navigate pages manually in jaeger-ui), so I dug up their raw SVG path from node_modules and then took a snip of their rendered version using SVG Viewer

Feel free to suggest any alternate icon, in case you feel some icon is not looking good. You may use this to surf all of react icons.

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (843d88e) 96.01% compared to head (378a4ae) 96.01%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1721   +/-   ##
=======================================
  Coverage   96.01%   96.01%           
=======================================
  Files         241      241           
  Lines        7562     7562           
  Branches     1987     1987           
=======================================
  Hits         7261     7261           
  Misses        301      301           
Files Changed Coverage Δ
...ts/DeepDependencies/Graph/DdgNodeContent/index.tsx 100.00% <ø> (ø)
...components/DeepDependencies/Header/ChevronDown.tsx 100.00% <ø> (ø)
...i/src/components/DeepDependencies/Header/index.tsx 100.00% <ø> (ø)
...rc/components/DeepDependencies/SidePanel/index.tsx 100.00% <ø> (ø)
...r-ui/src/components/SearchTracePage/SearchForm.jsx 91.61% <ø> (ø)
...nents/SearchTracePage/SearchResults/ResultItem.tsx 100.00% <ø> (ø)
...mponents/TraceDiff/TraceDiffHeader/TraceHeader.tsx 100.00% <ø> (ø)
...ents/TracePage/TracePageHeader/TracePageHeader.tsx 97.56% <ø> (ø)
...s/TracePage/TracePageHeader/TracePageSearchBar.tsx 100.00% <ø> (ø)
...nents/TracePage/TraceTimelineViewer/SpanBarRow.tsx 100.00% <ø> (ø)
... and 12 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro
Copy link
Member

not exactly the scope of this PR, but I dislike that we use both antd/icons and react-icons, is there a strong reason to do that and not converge onto one?

@anshgoyalevil
Copy link
Member Author

anshgoyalevil commented Aug 22, 2023

As per my analysis so far, we can use react-icons in place of antd icons, but not vice versa.
Since react-icons offers the best of both worlds, and we too are using some icons which, I haven't looked exactly but I think are not available in antd icons, or atleast react-icons/ai

@yurishkuro
Copy link
Member

As per my analysis so far, we can use react-icons in place of antd icons, but not vice versa.

it's possible, although I am skeptical if we can't find close equivalents.

What I don't like about react-icons is that it's all over the place with all kinds of namespaces, like md, io, fa. As I understand, those are competing icon sets, presumably created with some design consistency principles, but we keep mixing them randomly.

@yurishkuro yurishkuro merged commit a01f444 into jaegertracing:main Aug 22, 2023
@anshgoyalevil
Copy link
Member Author

I agree.
Maybe in future, would have to put up some extra work in finding the alternatives.
Like we can stick to react-icons/ fa (font awesome) icons. As far as I know, they offer a wide range of icons.

@yurishkuro
Copy link
Member

I suggest we book a ticket and include a simple grep/awk output that lists all distinct icons we're using. I doubt it's more than a couple dozen.

@yurishkuro
Copy link
Member

#1723

@anshgoyalevil
Copy link
Member Author

Thanks for creating the ticket.
I would try finding the icons and prepare a gist.
In the meantime, someone from community can pick it up

@anshgoyalevil anshgoyalevil deleted the bump-qs branch August 23, 2023 12:56
@anshgoyalevil anshgoyalevil restored the bump-qs branch August 25, 2023 09:40
@anshgoyalevil anshgoyalevil deleted the bump-qs branch August 25, 2023 11:15
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.

2 participants