hasadna / open-bus-map-search

open-bus-map-search
https://open-bus-map-search.hasadna.org.il/dashboard
MIT License
48 stars 85 forks source link

refactor: replaced some Ant components with MUI components. #752

Closed SariHop closed 3 weeks ago

SariHop commented 1 month ago

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.

github-actions[bot] commented 1 month ago

Preview: https://s3.amazonaws.com/noam-gaash.co.il/9438827734/open-bus/0f12388af038e026475519a0f7901ab4ab9132ce/index.html Preview Storybook: https://s3.amazonaws.com/noam-gaash.co.il/9438827734/open-bus/0f12388af038e026475519a0f7901ab4ab9132ce/storybook/index.html

NoamGaash commented 1 month ago

Hi, thanks for your efforts! Please consider configuring eslint in your ide, or run npm run lint:fix to fix lint errors

SariHop commented 1 month ago

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.

shootermv commented 1 month ago

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 https://github.com/hasadna/open-bus-map-search/pull/752#issuecomment-2119124202, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKGMHX6ZLXNFVVIAKISNQLZDBDL5AVCNFSM6AAAAABH2WCCNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGEZDIMRQGI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

NoamGaash commented 1 month ago

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

SariHop commented 1 month ago

This is more information about my changes:

shootermv commented 1 month ago

one little problem i saw (not sure if it has something with the change...) there are 2 dollar icons at the topbar image

NoamGaash commented 1 month ago

@shootermv Great catch! I guess we really like money here :stuck_out_tongue_winking_eye:

It's not related to this pull request but it's something we should fix. Do you want to open a pull request?

shootermv commented 1 month ago

Thanks! Not sure about PR, but I will open an issue.

SariHop commented 1 month ago

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

NoamGaash commented 1 month ago

@SariHop Thanks!