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

305 navcomp refactoring #348

Closed
wants to merge 7 commits into from

Conversation

san-oop
Copy link
Contributor

@san-oop san-oop commented Oct 17, 2020

Fixes #305

The single big "NavigationBarAndDrawer" component is now refactored in to smaller sub components; It is mainly divided in to 3 more components

  1. NavigationBar : The app bar
  2. NavigationDrawer: The hamburger menu
  3. NavigationPopover: Popovers for notifications which come in the app bar

NavigationDrawer is designed in such a way that an array of menuitems is looped to get each menu item. So if a new menu item is required to be added in future, it is as easy by just adding a new object to this array of menuitems. (Menuitems is further divided in to 2, primary and secondary: refer the code in NavigationDrawer)

NavigationPopover This is not a generic popover component, and didnt find any particular code saving or code reusability if a generic one is implemented( MUI popover is more generic already). Hence this popover is exclusively designed to be shown in the app bar(for notifications)

@AlbertoPdRF AlbertoPdRF added enhancement Related to improvements of existing features and/or addition of new ones front end Requires work on the front end of the application refactor Improve the code of existing features labels Oct 18, 2020
@AlbertoPdRF
Copy link
Owner

Awesome work @san-oop! 🤩

Before diving into the code details, I have a few high level comments:

  1. It seems like the pre-commit hook didn't work on your end, as there are a lot of code style issues. To make it work for future commits, on the client's directory, you have to delete the node_modules folder and run npm install again. To fix the existing issues, and also on the client's directory, you can run npx prettier --single-quote -w * and commit the resulting changes
    • Related to this, if you use the VS Code editor to code, the workspace configuration on the root of the repository should also help preventing these issues.
  2. I think we can rename the current Navigation component to something like Router, and then rename the higher order component NavigationBarAndDrawer to just Navigation, thus matching the pattern of other sets of components throughout the code base
  3. The locale files also need to be refactored to reflect the changes done on the components

After this is taken care of, we can discuss the details of the implementation! 🙂

Thanks for taking this one on! 💯

@AlbertoPdRF
Copy link
Owner

Hi @san-oop!

I've seen that there's a bug: when you try to scroll down, the app crashes. The bug is related to the HideOnScroll function. I believe that the best way to fix this would be to merge the new Navigation and NavigationBar components into one (let's name it NavigationBar, this way we can rename the new Router component back to Navigation). Doing this is esentially going a step backwards in the refactoring, by just extracting the drawer and the popovers from the current NavigationBarAndDrawer component, but I think it will be better as things will be easier this way.

Also, please don't forget to refactor the locale files too.

Let me know if you need help with anything! 🙂

@AlbertoPdRF
Copy link
Owner

Hey @san-oop! Sorry for not having answered for so long. I won't maintain Tisn anymore (see #449), so I'm closing this PR.

Thank you so much for having contributed to this project, it was so nice working with you!

@AlbertoPdRF AlbertoPdRF closed this Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Related to improvements of existing features and/or addition of new ones front end Requires work on the front end of the application refactor Improve the code of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor de NavigationBarAndDrawer component
2 participants