-
Notifications
You must be signed in to change notification settings - Fork 96
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
refactor: replaced some Ant components with MUI components. #752
Conversation
Hi, thanks for your efforts! |
I ran the command |
I think you should be more pesific about - which components you are about
to replace ("some" is too general). my opinion - it would be even better to
replace only one component per step...
…On Sun, May 19, 2024, 09:48 Sari H ***@***.***> wrote:
I ran the command npm run lint:fix.
This is my first PR, so I would like to know if there is anything else I
should do or know
before I commit the changes.
—
Reply to this email directly, view it on GitHub
<#752 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKGMHX6ZLXNFVVIAKISNQLZDBDL5AVCNFSM6AAAAABH2WCCNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGEZDIMRQGI>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Hi @SariHop , it's always good to commit any improvement. Right now, it's hard to check your pull request as it have many warnings on it. It is recommended to solve all lint errors before asking for a code review |
This is more information about my changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
I did notice the size of your headers is a little different. Is it on purpose? Are we ok with it?
@shootermv Great catch! I guess we really like money here 😜 It's not related to this pull request but it's something we should fix. Do you want to open a pull request? |
Thanks! Not sure about PR, but I will open an issue. |
The differences between MUI and Ant's text header size are quite significant. I found this size to be the most suitable so that the title would be noticeable and readable. @NoamGaash |
@SariHop Thanks! |
…ngle_design_system
Description
#158
Started to replace Ant components with MUI components. There is more progress to be made, but I would like to get feedback on my initial work.