Andrewnt219 / rent-near-me

https://rent-near-me.vercel.app/
1 stars 1 forks source link

UI: `Tab` and `HorizontalScroll` #118

Closed vitokhangnguyen closed 2 years ago

vitokhangnguyen commented 3 years ago

Resolving #78

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/andrewnt219/rent-near-me/832aZntG9wgAPcpFqkjwZRLvAXf1
✅ Preview: https://rent-near-me-git-vitokhangnguyen-issue78tab-andrewnt219.vercel.app

Andrewnt219 commented 3 years ago

Missing comments and change list. Those are helpful for reviewing and I don't have to question the implementation every time.

Separate refactor and feature next time btw. When there're 38 files pending for reviewing, I just feel too lazy to review.

vitokhangnguyen commented 2 years ago

Missing comments and change list. Those are helpful for reviewing and I don't have to question the implementation every time.

Comments updated. Change list: Add Tab-related components and HorizontalScoll components... like the title indicates.

Separate refactor and feature next time btw. When there're 38 files pending for reviewing, I just feel too lazy to review.

It's really hard to do this because they're tiny changes here and there that don't fit into a PR. More than 2/3 of those 38 files (39 now) are just stuff you can just take a glance and know exactly what changed.

For this PR specifically, I recommend fetching the branch and reviewing the files in @ui/Tab and @ui/HorizontalScroll. Everything else is just trivial stuff. You can test the Tabs and the HorizontalScroll at /account/security.

Andrewnt219 commented 2 years ago

It's really hard to do this because they're tiny changes here and there that don't fit into a PR. More than 2/3 of those 38 files (39 now) are just stuff you can just take a glance and know exactly what changed.

But don't do that the same with a new feature or a big refactor. It added noises and harder to review.

Comments updated. Change list: Add Tab-related components and HorizontalScoll components... like the title indicates.

Pretty sure there are more than just adding those two components...

vitokhangnguyen commented 2 years ago

But don't do that the same with a new feature or a big refactor. It added noises and harder to review.

Will keep that in mind in the future

Pretty sure there are more than just adding those two components...

The Tab itself is not a single component. as you can see here:

(btw, the use of index.ts here is justified because we only want to export the Tab, TabGroup and useTabGroup for outside consumer, other components are for internal use)

image

HorizontalScroll is one component.

Other changes:

Andrewnt219 commented 2 years ago

Why export hooks as default? Hooks' name should be the same everywhere.

vitokhangnguyen commented 2 years ago

Why export hooks as default? Hooks' name should be the same everywhere.

The rule should be that if the file is dedicated to that function, component or what ever, it should be exported as default. No?

The file name is useClickOutside.tsx, then naturally, the hook should be exported default from that file.

vitokhangnguyen commented 2 years ago

Now I notice that other hooks are named exported as well. I can take this out from this PR but it should be fixed in another PR. Make no sense if trhe file is already dedicated to the hook but it's still named exported.