search

freeCodeCamp / pantry-for-good

An open source food bank logistics and inventory management tool
Other
395 stars 189 forks source link

Collapse navbar menus with only one item #329

Closed wacii closed 6 years ago

wacii commented 6 years ago

When a not-admin is logged in, the customer, donor and volunteer menus can have only one item, apply. These are replaced with a link reading something like 'Apply as a donor'.

The menus were not just unnecessary in these circumstances, but made the application more difficult to use. After a new user registered they would have to search for the next step rather than having it presented to them.

The change itself shouldn't have been difficult, but the way the menus were being generated was needlessly complicated, with work that should have been done in the view/components being done in the reducer: so that needed to be rewritten first. Specifically, the logic in the menu reducer was moved into the admin sidebar and client navbar components.

Additionally, I leveraged React Router to determine if certain links were active instead of the more involved custom approach used previously.

kenjiO commented 6 years ago

I like your idea of simplifying these things. @jspaine is the one who wrote the functionality so I would like to see if he approves before going any further since he created these menus and might have some reason to keep it the way it is. If he is OK doing it this way I would suggest changing the name of two components. Change MainMenuItem to MenuGroup and change SubMenuItem to MenuItem. I think it makes it more clear that way. It would probably be good to define each component in it's own file too. Also for the treeview menu items, the arrow showing if they are expanded or collapsed is not showing up anymore.

wacii commented 6 years ago

Oh right, the arrow. I forgot to fix that.

I definitely agree about the naming. Thought it might make the review easier, having the new code in the old files, but that was probably just me being lazy. I'll give it some thought.

jspaine commented 6 years ago

Only had a quick look so far, but yeah it seems really good and much simpler!

wacii commented 6 years ago

Fixed the arrow, revisited naming, and made sure it was one component to a file.