-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
feat: CalendarSettings
atom
#16120
feat: CalendarSettings
atom
#16120
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Graphite Automations"Add foundation team as reviewer" took an action on this PR • (08/07/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add platform team as reviewer" took an action on this PR • (08/07/24)1 reviewer was added to this PR based on Keith Williams's automation. |
packages/platform/atoms/calendar-settings/wrappers/CalendarSettingsWebWrapper.tsx
Show resolved
Hide resolved
@Ryukemeister looks like this import statement in
I'm not sure why this happens. By any chance do you know @ThyMinimalDev? I updated the import statement in this commit I also noticed some build errors so I converted some components into client components. It should cause any problems. |
Thinking back to this I think we can create a follow up PR to remove any instances of |
onDeletionConfirmation={async () => { | ||
slug && | ||
(await deleteCalendarCredentials({ | ||
calendar: slug.split("-")[0], |
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.
NIT: slug.split("-")[0]
could be validated to check if its equal to google, apple calendar or ms outlook slug and if not PlatformDisconnectIntegration
throws error.
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.
Agreed, good catch!
<CalendarSettingsHeading /> | ||
<List noBorderTreatment className="p-6 pt-2"> | ||
{data.connectedCalendars.map((connectedCalendar) => { | ||
if (!!connectedCalendar.calendars) { |
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.
NIT: I think we also need to check if connectedCalendar.calendars length is bigger than 0 because its an array that could theoretically be empty
); | ||
}; | ||
|
||
const PlatformDisconnectIntegration = (props: { |
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.
Would be good to split small components from this file into separate files.
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.
Since the smaller components are not being used anywhere else i.e. since they are exclusive to only the Platform wrappers thats why me and @ThyMinimalDev agreed its best to keep them in the same file itself.
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.
these components, at least currently don't really exist without each other, keeping in the same file for now should not be an issue
}); | ||
|
||
return ( | ||
<DisconnectIntegrationComponent |
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.
In prod web app disconnect integration looks like:
but in atoms the padding looks very big in examples app:
So it would be good to add className to DisconnectIntegrationComponent
and in web we pass existing padding and in atoms we pass smaller padding also given that in atoms the warning icon is not displayed so we need less space.
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.
@supalarry I think the padding looks very big because the danger icon that you see in the web app dialog is missing and thats what causes it. Otherwise both the dialogs have the same styles, can confirm that. But its always good to a add classnames prop.
return ( | ||
<DisconnectIntegrationComponent | ||
onDeletionConfirmation={async () => { | ||
slug && |
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.
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.
- Icons not appearing is a known issue that is caused by perf: use svg sprite for icons #16135, we also have a discussion in threads going on here so that should be resolved soon
- When you delete a calendar, it triggers the custom hook to delete the calendar credentials which takes a few seconds although the toast appears instantly. But since the web app follows the same behaviour thats why I implemented it the same way in the atom. The custom hook also has logic to invalidate connected calendar queries, so it refetches the connected calendars once the delete request is successful so technically the deleted calendar should be removed automatically without refreshing.
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.
Great job Rajiv! I confirm that i can connect 2 calendars and that they availabilities are checked correctly when booking. However:
- Remove calendar icon is not showing up, and when calendar is removed message of removal appears but it is still there unless page is refreshed.
- Added other comments
* dumb components for calendar settings * shadcn switch * update exports * update packages * add calendar settings atom to examples app * init calendar settings atom * refactors * fix import path * export type for calendar switch props * invalidate queries on deleting calendar credentials * replace calendars list with calendar settings wrapper * cleanup * update styling * refactors * cleanup * fix: missing key prop CalendarSettingsPlatformWrapper * Label as client components * Address client component build errors * Move QueryCell out of packages/lib * PR feedback * more feedback --------- Co-authored-by: Joe Au-Yeung <65426560+joeauyeung@users.noreply.github.com> Co-authored-by: Morgan <33722304+ThyMinimalDev@users.noreply.github.com> Co-authored-by: Morgan Vernay <morgan@cal.com> Co-authored-by: Joe Au-Yeung <j.auyeung419@gmail.com>
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
This can be tested in the examples that we have for atoms which is under
/packages/platform/examples/base
. Navigate to the/calendars
page in the examples app