Skip to content

Fix destination calendar overflow on installed and move DestinationCalendarSelector to feature package#4778

Merged
joeauyeung merged 31 commits intomainfrom
hotfix/install-calendar-overflow
Oct 18, 2022
Merged

Fix destination calendar overflow on installed and move DestinationCalendarSelector to feature package#4778
joeauyeung merged 31 commits intomainfrom
hotfix/install-calendar-overflow

Conversation

@joeauyeung
Copy link
Copy Markdown
Contributor

What does this PR do?

This PR fixes the destination calendar overflowing when the ID was too long. I have also moved the DestinationCalendarSelector to the feature/calendar package.

Before
image

After
image

Fixes # (issue)

Environment: Staging(main branch) / Production

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

  • Change the destination calendar in the following places
    • Installed
    • A single event type
    • Settings -> Calendars

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works
  • I haven't checked if new and existing unit tests pass locally with my changes

@vercel
Copy link
Copy Markdown

vercel Bot commented Sep 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Oct 18, 2022 at 6:04PM (UTC)

@leog
Copy link
Copy Markdown
Contributor

leog commented Sep 30, 2022

image

@joeauyeung That looks a bit off, as the combobox now doesn't include details of the calendar (to avoid a long ID overflowing the UI), it now looks too big for its content. Maybe a better solution would be to truncate that ID, as the idea is to provide a way to identify which calendar provider was selected aside from the name.

Also, the long ID comes from a CalDav calendar right? When a Google Calendar is selected, the Google email associated to the Google Calendar should show up as the ID of that Google Calendar. The goal here is, as you may have multiple Google Calendars and in the case you have identical calendars in them, you couldn't know which was selected as destination.

@joeauyeung
Copy link
Copy Markdown
Contributor Author

@leog what if we render the integration headers that are present when choosing a destination calendar? Here's a Loom with my thoughts

https://www.loom.com/share/d7664d33a53b40c5b7e26c3754772d46

@leog
Copy link
Copy Markdown
Contributor

leog commented Oct 1, 2022

@leog what if we render the integration headers that are present when choosing a destination calendar? Here's a Loom with my thoughts

https://www.loom.com/share/d7664d33a53b40c5b7e26c3754772d46

Very well thought, I like it 👍 I wonder if we can improve that lag when loading the name to make it perfect. Anyway, I think it works, we could ask @Jaibles just to confirm.

@joeauyeung joeauyeung added the 🎨 needs design Before engineering kick-off, a designer needs to submit a mockup label Oct 1, 2022
@ciaranha
Copy link
Copy Markdown
Member

@joeauyeung @leog this looks good to me in the Loom!

Would it be awkward/weird to remove the "calendar" in these particular instances just to shorten this?
CleanShot 2022-10-11 at 17 56 36@2x

I know technically it's correct to leave it in, to match the app name but it does really make the labelling longer and it's obvious here it's a calendar.

@joeauyeung joeauyeung removed the 🎨 needs design Before engineering kick-off, a designer needs to submit a mockup label Oct 12, 2022
Copy link
Copy Markdown
Contributor

@leog leog left a comment

Choose a reason for hiding this comment

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

Looks good to me! Nice work Joe

Comment thread apps/web/pages/settings/my-account/calendars.tsx Outdated
Copy link
Copy Markdown
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +6 to +9
name: "Outlook Calendar",
description: _package.description,
type: "office365_calendar",
title: "Office 365 / Outlook.com Calendar",
title: "Outlook Calendar",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be another PR but I'll allow it

@zomars
Copy link
Copy Markdown
Contributor

zomars commented Oct 18, 2022

We do have some type-check errors tho.

@joeauyeung joeauyeung enabled auto-merge (squash) October 18, 2022 18:06
@joeauyeung joeauyeung merged commit 5ab5af7 into main Oct 18, 2022
@joeauyeung joeauyeung deleted the hotfix/install-calendar-overflow branch October 18, 2022 18:06
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
…lendarSelector to feature package (calcom#4778)

* Standardize destination calendar selector

* Move DestinationCalendarSelector to feature package

* Render integration name

* Add custom components to label and selected

* Render destinationCalendar on page load

* Change name to just Outlook

* Small fixes

* Merge branch 'main' into hotfix/install-calendar-overflow

* Merge branch 'main' into hotfix/install-calendar-overflow

* Fix type errors

* Fix type errors

* Update apps/web/pages/settings/my-account/calendars.tsx

* More type fixes

Co-authored-by: Omar López <zomars@me.com>
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
…lendarSelector to feature package (calcom#4778)

* Standardize destination calendar selector

* Move DestinationCalendarSelector to feature package

* Render integration name

* Add custom components to label and selected

* Render destinationCalendar on page load

* Change name to just Outlook

* Small fixes

* Merge branch 'main' into hotfix/install-calendar-overflow

* Merge branch 'main' into hotfix/install-calendar-overflow

* Fix type errors

* Fix type errors

* Update apps/web/pages/settings/my-account/calendars.tsx

* More type fixes

Co-authored-by: Omar López <zomars@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants