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

✨ improved the UX for the right sidebar #63

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

AzharAhmed-bot
Copy link
Contributor

@AzharAhmed-bot AzharAhmed-bot commented Jan 23, 2024

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

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

##Screenshots
Screenshot 2024-01-23 214029

@tyler-dane
Copy link
Contributor

tyler-dane commented Jan 23, 2024

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.

@AzharAhmed-bot
Copy link
Contributor Author

Hey, no worried. Let me look into it 👍

@AzharAhmed-bot
Copy link
Contributor Author

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

@tyler-dane
Copy link
Contributor

tyler-dane commented Mar 9, 2024

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 tyler-dane added the enhancement New feature or request label Mar 10, 2024
@tyler-dane tyler-dane marked this pull request as draft March 23, 2024 01:58
@tyler-dane
Copy link
Contributor

Converted to draft due to the issues mentioned above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🎨 Improve the UX for the right navbar
2 participants