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

Enable toggle in and out of calendars on small screens #1584

Merged
merged 26 commits into from
Dec 12, 2024

Conversation

DianaXWiki
Copy link
Contributor

No description provided.

@DianaXWiki DianaXWiki added the Ready to Review This PR is ready to be checked by another team member label Aug 5, 2024
@davidbenque davidbenque added this to the Autumn release (2024.9.0) milestone Aug 8, 2024
Copy link
Contributor

@davidbenque davidbenque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ability to collapse the calendar list improves this UI on mobile, but now the list is very hard to discover. I think a button should be added when the list is collapsed, see the very quick mockup

image

@davidbenque davidbenque removed the Ready to Review This PR is ready to be checked by another team member label Aug 8, 2024
@DianaXWiki DianaXWiki marked this pull request as ready for review August 9, 2024 14:44
@DianaXWiki DianaXWiki added Ready to Review This PR is ready to be checked by another team member Ready to Test This PR is ready to be tested labels Aug 9, 2024
@davidbenque davidbenque removed the Ready to Test This PR is ready to be tested label Aug 20, 2024
@davidbenque
Copy link
Contributor

davidbenque commented Aug 23, 2024

It is unclear how/when the "Show Calendars" button is triggered.

  • When manually decreasing the screen width (e.g. in Firefox responsive view) the button does not get added when moving to a small size
  • When manually increasing the screen width the button stays in desktop mode but does not do anything when clicked

@davidbenque
Copy link
Contributor

davidbenque commented Aug 23, 2024

  • "Show Calendars" should be changed to "Hide Calendars" when the list is shown

@davidbenque davidbenque removed the Ready to Review This PR is ready to be checked by another team member label Aug 28, 2024
@DianaXWiki DianaXWiki added the Ready to Review This PR is ready to be checked by another team member label Aug 29, 2024
Copy link
Contributor

@davidbenque davidbenque left a 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

@davidbenque davidbenque dismissed their stale review August 29, 2024 13:11

changes have been made

yflory
yflory previously requested changes Sep 2, 2024
Copy link
Contributor

@yflory yflory left a 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.

www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
@yflory yflory removed the Ready to Review This PR is ready to be checked by another team member label Sep 2, 2024
@DianaXWiki DianaXWiki added the Ready to Review This PR is ready to be checked by another team member label Sep 30, 2024
www/calendar/app-calendar.less Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
@davidbenque davidbenque dismissed yflory’s stale review October 17, 2024 15:27

review is out of date, new review needed

@yflory yflory removed the Ready to Review This PR is ready to be checked by another team member label Dec 3, 2024
Copy link
Contributor

@yflory yflory left a 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

www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Show resolved Hide resolved
www/calendar/inner.js Show resolved Hide resolved
www/calendar/inner.js Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
www/calendar/inner.js Outdated Show resolved Hide resolved
@DianaXWiki DianaXWiki added the Ready to Review This PR is ready to be checked by another team member label Dec 5, 2024
@yflory yflory added Ready to Test This PR is ready to be tested and removed Ready to Review This PR is ready to be checked by another team member labels Dec 9, 2024
@yflory yflory merged commit 5ddfe50 into staging Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to Test This PR is ready to be tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants