navgurukul / bhanwari-devi

Frontend Repository for Merakilearn maintained by Navgurukul students and volunteers
https://www.merakilearn.org/
31 stars 17 forks source link

"On Hover" not working as expected #703

Open saquibaijaz opened 1 year ago

saquibaijaz commented 1 year ago

To Reproduce

  1. User hover's over an item in the navbar
  2. The menu is not opening properly and the screen is shaking
  3. While the dropdown is still open, user hover's over another menu item
  4. The other dropdown does not open unless the previous one is collapsed

Expected behavior The dropdowns should open for the item which is Hovered On

Screencast https://drive.google.com/file/d/1vciHVbLcVHYU6zvemZFYk2IMqh3sEgCa/view?usp=sharing

https://drive.google.com/file/d/14AB3a1zYsRrZmOP3WqpdfwUE0LApWIOl/view?usp=sharing

Refer Sheet TC for Landing page without login (website), Test cases 13.1 and 13.2

kartiks26 commented 1 year ago

@jschanker i tried solving this issue by image Adding a onMouseLeave to the menuItem but as they are both in different container it doesn't seem to work in perfect way

jschanker commented 1 year ago

@kartiks26 Will try to look at this at the end of the week when I have some more time.

kartiks26 commented 1 year ago

@jschanker Okay

jschanker commented 1 year ago

@kartiks26 I noticed that the problem was that the other menus were being covered so onMouseEnter didn't work. Removing fixed positioning works. Again, don't really have time to work on this right now, but this can get you started with your investigation. Also, see https://stackoverflow.com/questions/55318477/how-to-make-material-ui-menu-based-on-hover-not-click image

saquibaijaz commented 1 year ago

@kartiks26 , not working

komalahire commented 1 year ago

@saquibaijaz This issue has been done. Please check.

saquibaijaz commented 1 year ago

@komalahire Not working as expected

https://user-images.githubusercontent.com/111858763/193828198-8c8aafb9-500b-4281-a884-fb105fdb52a2.mp4

komalahire commented 1 year ago

Ok, let me check again.

Poonam-Singh-Bagh commented 1 year ago

@jschanker can you please fix it?

jschanker commented 1 year ago

@Poonam-Singh-Bagh @kartiks26 @saquibaijaz To give an update on this, the issue for the Learn/About/Get Involved dropdowns is fixed by https://github.com/navgurukul/bhanwari-devi/pull/740. You can see that here: https://bhanwari-devi-kt6o3pt7s-navgurukul-meraki.vercel.app/ . The strategy I settled on for now was to have inDropdown state that gets set to false when the mouse leaves the navigation menu item or dropdown and gets set to true when one of these is entered. If it's false for around 200 milliseconds, the menu is closed. (This addresses the two-container issue that Kartik mentioned.) This took me longer than I expected to fix, primarily because I wasn't aware of this behavior: https://medium.com/programming-essentials/how-to-access-the-state-in-settimeout-inside-a-react-function-component-39a9f031c76f#02f8. I think it might be better to nest the dropdown inside the menu item but for now, the current approach should be sufficient.

But the problem currently still exists for the User and switch roles dropdown. Also, we disabled the hover effect for touch screens for the switch views dropdown but it still persists for the user dropdown and the Learn/About/Get Involved dropdowns.

In short, we should be using a reusable dropdown component that handles all of these issues so that we only need to change it in one place when it needs fixes or better implementations. I need to prepare for classes again for this week so I'll come back to this next weekend.

saquibaijaz commented 1 year ago

@Poonam-Singh-Bagh @kartiks26 Please also check the "scroll bar" in the dropdown displayed when we hover on "Learn".

Poonam-Singh-Bagh commented 1 year ago

@jschanker It's not working properly when the user is logged in. I guess when the mouse leaves the navigation inDropdown state isn't getting set to false.

Screenshot from 2022-10-17 21-58-58

jschanker commented 1 year ago

@Poonam-Singh-Bagh Unfortunately, this is because of some semi-duplicated code. I made the correction for the public learn dropdown in https://github.com/navgurukul/bhanwari-devi/blob/fix/hover-menu-close-on-leave/src/components/Header/index.js#L91-L147, but not for the learn menu for the student header dropdown https://github.com/navgurukul/bhanwari-devi/blob/fix/hover-menu-close-on-leave/src/components/Header/StudentHeader/index.js#L61-L72.

kartiks26 commented 1 year ago
jschanker commented 1 year ago

@amansharmma I've returned to this, but I have to think how to best factor out the commonality.

Poonam-Singh-Bagh commented 1 year ago

@jschanker are you working on it?

jschanker commented 1 year ago

@Poonam-Singh-Bagh Yes, I'm starting work on this issue again.

jschanker commented 1 year ago

@Poonam-Singh-Bagh @saquibaijaz Now fixed to use common DropDownMenu and TextButtonDropDownMenu components for User/Switch Roles menus (but not deployed to dev server, available at https://bhanwari-devi-mc9iqsoyx-navgurukul-meraki.vercel.app/). Works for the sidebar navigation with the Switch Views as well although there's no left margin for the dropdown in this case. Still need to incorporate for Student Dropdown when logged in. It has been challenging to work around the Menu's presentation layer that covers the button.

Also, right now with the edit, the user can move along the options and have the dropdowns momentarily appear one above the other because there's a delay before they close. We can fix this by adjusting the delay or by setting only one to be open at any time.

saquibaijaz commented 1 year ago

@jschanker we need to fix this scrollbar at the side of the dropdown Cc: @Poonam-Singh-Bagh hover

jschanker commented 1 year ago

@saquibaijaz Related to this is https://github.com/navgurukul/bhanwari-devi/issues/803 where the user menu also has a scrollbar. When it's in the sidebar, do we want it to be in the page flow and expand/contract on clicks only regardless of whether the device is a touchscreen or not? This would be for consistency with the Learn menu.

The Change Roles/Public menu dropdowns aren't consistent in the sidebar. The public ones gradually show/hide their dropdowns (accordion) on clicks but not on hovers and stay in the flow of the sidebar. The change roles view one appears in its own dropdown modal and also responds to hovers. This is because the public menus are handled with different components when the sidebar is open (MobileDropdown) whereas the ChangeRolesView menu uses the Dropdown regardless of whether it appears at the top or in the sidebar. image

saquibaijaz commented 1 year ago

@jschanker It should be like this and should be everywhere. No Scrollbar.

https://user-images.githubusercontent.com/111858763/203553853-2651bfdd-f129-4e53-94ba-80ed11f740ed.mp4

jschanker commented 1 year ago

@jschanker we need to fix this scrollbar at the side of the dropdown Cc: @Poonam-Singh-Bagh hover

Scrollbar issue fixed. The vertical padding on the inline a element was causing the issue. Continuing to fix other issues.

jschanker commented 1 year ago

@saquibaijaz There might be some small changes that are needed to PR #740 such as decreasing the delay for a menu to close so they don't appear on top of each other or adjusting the margin, but I think it basically fixes the code to meet the requirements. You can preview it here: https://bhanwari-devi-ehthh0y8u-navgurukul-meraki.vercel.app/ . The scrollbar will appear only when necessary now, and the dropdown menus in the drawer all appear as accordion ones. CC: @komalahire @Poonam-Singh-Bagh

saquibaijaz commented 1 year ago

@jschanker The hover is working as expected. The only problem is with the scrollbar appearing and disappearing on the page while we hover over the items.

Check the attached screencast.

https://user-images.githubusercontent.com/111858763/206199303-df1149f1-9fec-4e04-aa48-89abb1b46fea.mp4

jschanker commented 1 year ago

@saquibaijaz Seems to be another issue with the Menu component (https://github.com/mui/material-ui/issues/5185). I tried resolving this with disableScrollLock but it didn't redeploy on Vercel for some reason so I'm not sure if it worked.

jschanker commented 1 year ago

@saquibaijaz It eventually deployed. Maybe there were other deployments in progress holding this one up. Seems to work now: https://bhanwari-devi-5dczeisd7-navgurukul-meraki.vercel.app/

saquibaijaz commented 1 year ago

@jschanker I checked for all the roles. It's working perfectly

@Poonam-Singh-Bagh @komalahire You guys can merge this

komalahire commented 1 year ago

Everything is working fine.
Thanks @jschanker