chingu-voyages / v24-geckos-team-02

Books Plus | Voyage-24 | https://chingu.io/
GNU General Public License v3.0
1 stars 1 forks source link

Adding hamburger menu & responsiveness on longer screen devices #74

Closed snrelghgub closed 3 years ago

snrelghgub commented 3 years ago

As per our previous discussions & to handle important issues raised in our last two meetings, I'm making a PR to include the following new priority changes:

jdmedlock commented 3 years ago

In 11 November team meeting it was felt that the Login button should be a text link instead so it will match the styling of the other links in the menu. This clash between styles is especially apparent in the Hamburger menu. Arun will change this in another PR.

Arun will also update the Login link in his PR to correct the HTTP 400 error when attempting to login

snrelghgub commented 3 years ago

@jdmedlock Thanks for the cordial update. I pulled the latest changes made to the development branch since yesterday & have also already made the necessary changes to complement the new menu options added since then. I also took the opportunity to review & tested various alignment issues with the card buttons in both mobile & desktop view to get them fixed & done in this PR. There was also a significant issue with the colour picked for the favourite icon displayed in each card, so I also updated to meet the minimal WCAG for graphical components & retain a reasonable UI/UX standard for our app. I have yet to resolve to merge conflicts for new changes made in the last hour, so I'm working on this now.

Guitarhub786 commented 3 years ago

Nice transformation from X to hamburger "logo".

1) Is it possible to add the ability for the user to click outside your "sidebar" for sidebar to go away.

2) The difference size text lengths of headings. May look more uniform if you "center" the text options. [search] [Favorites] [About] in the sidebar menu. i.e Text inside the textbox.

3) May also look more uniform if you "center" you textbox's inside your sidebar box. They look one-sided (to the right).

4) (Personal preference) Is there an advantage of having a pop-up "box" sidebar or is there an obstruction for having a sliding sidebar?

5) (Personal preference) Have you also considered having the "icon" part of the logo (like the favicon) at the top of your sidebar?

mokokom commented 3 years ago

Nice work @snrelghgub! I also agree with @Guitarhub786 's suggestions. Also I think that orange borders on sidebar are maybe not necessary. Maybe the background should have a kind of light-grey opacity while the sidebar background stay white (as the modal). This way we don't need the orange border on sidebar ( in my opinion it's too much with the other orange border in the background that wrap the search/or favorites ) Capture d’écran 2020-11-14 à 05 40 54

snrelghgub commented 3 years ago

Thanks for your inputs & suggestions for improvements @Guitarhub786 @mokokom I took as much as I could onboard & what I think is reasonable change considering time constraints & here are the changes that I made since your last review: • I changed the text font size of the menu options in mobile view < 390px as I agree it was too big, but it still the same in browser view as it'd be too small & I think it is reasonable to keep the same aspect ratio  • I centered all the menu options inside of their menu container  • I centered the text of each menu option inside their sidebar box  • I added a sliding effect for when the user clicks on the hamburger icon to slide the menu options & incorporate your suggestion the best I could  • I don't think it's necessary to have an icon displayed since the logo is already displayed right to the hamburger menu options, but I have added enough new space so that the menu options do not cover the logo especially in mobile view  • I also added a light-blueish opacity background colour that is part of our colour scheme already instead of using grey which is not & was able to remove the borders without it negatively affecting the visuals  • Although not requested, I also refactored implemented code to return the hamburger menu from its own react component & tested that all the links, as well as login in & logging out, is still working fine

snrelghgub commented 3 years ago

Additional change: • User can now also click anywhere outside the hamburger menu to close it more conveniently without having to rely entirely on the close button displayed & after opening menu

Guitarhub786 commented 3 years ago

I have approved the PR so it can be merged into development. But when you can please remove unnecessary comment of redundant code that does not describe the code, please.