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

TabNavigator #17

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

TabNavigator #17

wants to merge 6 commits into from

Conversation

ManhHoDinh
Copy link
Collaborator

Hello Hellu

Copy link
Owner

@vuhoang-gr vuhoang-gr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Chung
  • Lý do xóa query-string trong package là gì thế?
  • Lý do xóa gần hết các icon có từ trước trong icons/index.js là gì thế?
  • src/components/Button/CustomButton.js là cái gì zợ? Nó là component global hay chỉ dùng cho cái màn hình nào đó ông làm thoi?
  1. PaymentNavigator
  • Tôi không chắc có nên tách riêng payment ra 1 route không, cái này tôi sẽ tìm hiểu thêm, nhưng cá nhân tôi nghĩ là không.
  • Lý do Auth và Main cần 2 luồng (routes) riêng, là vì khi đăng nhập thì nó không cần đến auth và khi đăng xuất thì nó không cần đến main.
  • Thế nhưng ở đây mình vẫn có thể back lại từ màn hình payment về màn hình cũ, nên nếu để 1 route riêng mình sẽ phải xử lý nó cũng dở phết.
  • Nhưng nah, nếu ông thích xử lý và vẫn để luồng riêng thì cũng oke thôi, nhưng nó sẽ không nằm trong NavigationContainer nhé, nó chỉ ở trong Stack.Navigator thôi. Vì như đã nói, NavigationContainer là cái bao ở ngoài, mình sẽ không cần thêm cái đấy ở đâu khác cả.
  1. RootNavigator
  • Tôi chưa thấy a bạn update cái này nhở? Không có update gì hay chưa push tek?
  1. Tab Navigator (Đọc thêm - Có thể sửa nếu thích hoặc không cũng ksao)
  • Nhận xét về mặt design: Có vẻ như text và icon của cái phần bottom tab đang không được thẳng hàng. Người dùng sẽ muốn nó thẳng hàng text với text và icon với icon ấy, ông có thể tìm hiểu thêm cách làm nhes.
  • Phần style đang bị lặp code khá nhiều, kiểu cái Tab.Screen nào cũng dùng chung 1 cái chỉ khác cái icon ấy. Ông có thể tìm hiểu thêm về HOC để không cần lặp code.

@ManhHoDinh
Copy link
Collaborator Author

  1. Chung
    Cái này sau khi merge pull về có thể tui chỉnh hoặc tải module lại nó mất, chứ tui không hề xóa ( hoặc trên cái master có sao tui tải về )
  2. Để tui sửa cái ý cuối.

@vuhoang-gr
Copy link
Owner

Okei, vậy check lại package rồi sửa gì thì tạo lại pr nhes, ông có thể close nhes : ))

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