compass icon indicating copy to clipboard operation
compass copied to clipboard

✨ improved the UX for the right sidebar

Open AzharAhmed-bot opened this issue 2 years ago • 5 comments

Description

As a user when I: - Click the hamburger menu it makes the difference when it is open and when it is closed .☰ for open and X for close . - Click somewhere else in the page the right side bar closes. - Press the esc button the right sidebar collapses.

This PR fixes #50

Checklist

  • [x] My code follows the style guidelines of this project
  • [x] I have performed a self-review of my code
  • [ ] My changes generate no new warnings

##Screenshots Screenshot 2024-01-23 214029

AzharAhmed-bot avatar Jan 23 '24 18:01 AzharAhmed-bot

Hey, thanks for taking the time to submit another PR.

I reviewed it locally and found a few things that would need to be addressed before merging:

  1. It's behind main

  2. It's triggering the ResizeObserverLoop error when devtools is open. This used only to happen occasionally, but now it happens whenever collapsing the menu. AFAIK, it's only an issue in dev, but seeing that runtime error hurts the developer experience, which I worry will dissuade others.

  3. Opening and collapsing the sidebar triggers API requests.

Having the X icon is a better UI experience, but I'm afraid the extra issues this PR introduces aren't worth that benefit. If they're resolved, I'd be happy to do another review

Screen Shot 2024-01-23 at 1 47 38 PM

~~ Its collapsing behavior should follow the same pattern as the left sidebar. Specifically, when the sidebar is open and a user clicks on the all-day grid, main grid, or left sidebar, then the right sidebar should simply collapse.~~ ~~Currently, the right sidebar collapses and the regular click action is created (starting a new event), which can be annoying if a user just wants to collapse the right sidebar~~

  • Removed this required, because it's already doing that in main. This can be addressed in a separate PR.

tyler-dane avatar Jan 23 '24 19:01 tyler-dane

Hey, no worried. Let me look into it 👍

AzharAhmed-bot avatar Jan 30 '24 17:01 AzharAhmed-bot

Hey Tyler, I have been looking into it. As you said earlier opening and closing the navbar calls a API request tell me more about it. Furthermore, I would like to know more about the ResizeObservable.

What's in my mind is that when the X button changes to ☰ , it causes a change in the size of DOM element which now occurs without letting the observer function know. I think all along this is why maybe you chose to let the ☰ stay in both open and close state. I feel like its a small logical error that I can fix but am struggling to identify it.

I'll appreciate if you could help me figure out what could be a possible solution to fix the undelivered notification. Thank you

AzharAhmed-bot avatar Mar 06 '24 10:03 AzharAhmed-bot

As you said earlier opening and closing the navbar calls a API request tell me more about it.

If you watch the backend logs, you'll see that opening and closing the sidebar triggers event requests. They'll look something like this:

200 GET /api/event?start=2024-03-03T00:00:00-06:00&end=2024-03-09T23:59:59-06:00 39.615ms Sat, 09 Mar 2024 23:33:34 GMT
200 GET /api/event?someday=true&start=2024-03-01&end=2024-03-31 43.044ms Sat, 09 Mar 2024 23:33:34 GMT
200 GET /api/event?someday=true&start=2024-03-01&end=2024-03-31 41.308ms Sat, 09 Mar 2024 23:33:36 GMT

Why that happens gets into how state is being accessed and updated throughout the app, along with the effect hooks you've created on this branch. I wish I could give a quick explainer, but it'd take a lot of debugging to confirm what exactly is triggering this behavior. It's potentially caused by poorly architected state management, your implementation of the effect hooks, or both.

Since this re-rendering and subsequent API requests will to slow down the UX, I'm afraid we can't proceed without fixing the underlying issues first.

I didn't get the chance to dig into the ResizeObservable error, but the above issue is already a show-stopper, so it's not worth worrying about that at this point.

tyler-dane avatar Mar 09 '24 23:03 tyler-dane

Converted to draft due to the issues mentioned above

tyler-dane avatar Mar 23 '24 01:03 tyler-dane