-
Notifications
You must be signed in to change notification settings - Fork 471
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
[deps] Choose between antd/icons and react-icons #1723
Comments
I tagged this as |
I would like to give it a try @yurishkuro. I'm new to this project and found this good first issue. |
Trying to follow the instructions to list the icons imported from both libraries:
It feels like FYI, below are the commands I'm trying to append with For grep -r "from '@ant-design/icons'" ./packages/jaeger-ui | awk -F '{|}' '{ gsub(/ /, "", $2); split($2, a, ","); for(i in a) print a[i] }' | sort | uniq -c For grep -r "from 'react-icons" ./packages/jaeger-ui | awk -F ' ' '{print $2}' | sort | uniq -c |
Pretty sure we're not using Smile. You need to grep in You also need to include |
Thx for pointing it out! Just adjusted the commands to focus on code in # @ant-design/icons
grep -r "from '@ant-design/icons'" ./packages/jaeger-ui/src ./packages/plexus/src | awk -F '{|}' '{ gsub(/ /, "", $2); split($2, a, ","); for(i in a) print a[i] }' | sort | uniq -c # react-icons
grep -r "from 'react-icons" ./packages/jaeger-ui/src ./packages/plexus/src | awk -F ' ' '{print $2}' | sort | uniq -c Here is the result I got:
|
@Kavinjsir for The |
@yurishkuro Will this work if we try to include the namespace like following? ❯ grep -r "from 'react-icons" ./packages/jaeger-ui/src ./packages/plexus/src | awk -F ' ' '{split($4,a,"/"); icon=a[4]; gsub(/;/, "", icon); namespace=a[3]; print namespace, $2}' | sort | uniq -c | sort -nr
6 io IoIosArrowDown
4 io IoIosArrowRight
4 io IoChevronDown
3 md MdVisibilityOff
3 io IoChevronRight
2 md MdVisibility
2 io IoHelp
2 io IoAndroidLocate
2 io IoAlert
2 fa FaFilter
1 ti TiCancel
1 md MdKeyboardArrowRight
1 md MdKeyboardArrowDown
1 md MdInfoOutline
1 md MdFileUpload
1 md MdExitToApp
1 io IoNetwork
1 io IoIosSearch
1 io IoIosFilingOutline
1 io IoArrowRightA
1 io IoAndroidOpen
1 io IoAndroidArrowBack
1 fa SortAmountAsc
1 fa FaTrash
1 fa FaCheck Seems like we haven't applied |
I'd like to recommend Google Fonts - Material Icons I have not included some of the Here is a table to represent the matching icons I found in google fonts
|
My suggesting would be to converge all icons on react-icons/io family, since this is the family already used the most in Jaeger. |
@yurishkuro I create a document which contains all icons set that are being used in code base and their io5 alternatives as well. But there are some icons that I'm not able to find in io5 set, Could you please take a look upon the doc? https://docs.google.com/document/d/15KcGEMfOCl1AmyhMI5T0EuYYwLZz-pGEibiU2uS87Dw/edit?usp=sharing |
I don't quite follow the layout of your doc, esp. when some rows have more than 2 icons. It would be better to have these columns:
|
the icon name is especially important as otherwise it's very difficult to make your doc actionable - can't search the code for a picture. |
Got it! Let me fix that. |
@yurishkuro Now I provide as much details as I can, But the table stretch quite a bit but it make sense now. I put icons in the columns to their corresponding family name. I hope it make sense to you as well |
@priyanshu-kun looks good (I don't think you needed so many columns in the table, the current family is obvious from the icon name). I left questions on some icons. Do you want to start making changes by swapping the icons where there's no concern? |
@yurishkuro Yeah I would love do that! And I make some changes in doc |
Signed-off-by: Priyanshu Sharma <[email protected]>
Signed-off-by: Priyanshu Sharma <[email protected]>
Signed-off-by: priyanshu-kun <[email protected]>
…hu-kun/jaegertracing#1723 Signed-off-by: priyanshu-kun <[email protected]>
Signed-off-by: priyanshu-kun <[email protected]>
Signed-off-by: priyanshu-kun <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Priyanshu Sharma [[email protected]](mailto:[email protected]) ## Which problem is this PR solving? - Resolves #1723 ## Description of the changes - In this pull request, I have replaced all uncontroversial icons from various icon sets with the io5 icon set, enhancing the visual consistency and user experience across the platform. ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: priyanshu-kun <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
@priyanshu-kun after #1771 merged, I am seeing these as remaining places. Does that match your understanding?
|
Actually, I missed that |
Let's pick another family then for the remaining ones, preferably with MIT license. |
Signed-off-by: priyanshu-kun <[email protected]>
Signed-off-by: priyanshu-kun <[email protected]>
## Which problem is this PR solving? Resolves #1723 ## Description of the changes - In this PR, I refactor an unused icon which I missed in last PR. --------- Signed-off-by: priyanshu-kun <[email protected]>
Follow up on this discussion #1721 (comment)
We are probably using about 10-20 different icons in the project, pulled rather randomly from antd and react-icons (and within react-icons from completely different sets).
The proposal is to consolidate onto a single set, for example react-icons/fa (assuming it can accommodate our existing usage).
The text was updated successfully, but these errors were encountered: