-
-
Notifications
You must be signed in to change notification settings - Fork 656
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
Enable toggle in and out of calendars on small screens #1584
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unclear how/when the "Show Calendars" button is triggered.
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems OK from the UI side. Needs a proper code review
…/cryptpad/cryptpad into multiple-calendars-inout-of-view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "temporary calendar" case is not working and was probably not tested: we don't see the calendar name and can't do anything with it.
In general, I don't think the way this issue has been handled is good enough:
- In the case where only one calendar is present, we're replacing this calendar with a button taking the same space. Clicking on this button adds the hidden entry, which makes this new system take the same space as 2 entries.
- This means that, with 1 or 2 calendars in your account or a team, the old system was better
- If a user has many teams with calendars, we end up in the same state than what we wanted to fix in the PR (see Should be able to toggle multiple calendars in/out of view #1371)
I think a better solution would be to partially or completely hide the list of calendar until the user clicks on a button. By "partially hiding", I'm thinking of something like the "teams" app where we only see the "icons" of the menu entries (the calendar colors in this case?) but it may also be better to keep the current menu and show or hide it with a button.
review is out of date, new review needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should be fixed and cleaned:
- Some elements from my previous review has been ignored
- The temporary calendar feature, which allows you to view a calendar shared by link, was broken in the last review and has now been completely removed
- Some smaller issues are still here
- A big part of the PR is about adding spaces, new lines and replacing classic functions by arrow functions. This should be removed from the PR to make the diff easier to read
- I'm not really sure it's important to change the view when the screen is resized but since it's done we might as well keep it (but I haven't tested it)
- I'm not sure why the PR adds some "aria" attributes to things not related to the initial issue
No description provided.