-
Notifications
You must be signed in to change notification settings - Fork 5
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(KNO-7528): add MsTeamsAuthButton #339
feat(KNO-7528): add MsTeamsAuthButton #339
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: c661d85 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@@ -9,6 +9,7 @@ | |||
"build:packages": "turbo build --filter=\"./packages/*\"", | |||
"dev": "turbo dev", | |||
"dev:next-example": "turbo dev --filter=\"./packages/*\" --filter=nextjs-example", | |||
"dev:packages": "turbo dev --filter=\"./packages/*\"", |
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.
Personally, I find this yarn run dev:packages
command useful, but I’m happy to remove it if others don’t find it useful.
|
||
export interface KnockMSTeamsProviderProps { | ||
knockMSTeamsChannelId: string; | ||
tenantId: string; |
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 corresponding prop in SlackKit is named tenant
rather than tenantId
:
javascript/packages/react-core/src/modules/slack/context/KnockSlackProvider.tsx
Lines 23 to 26 in 6d27a00
export interface KnockSlackProviderProps { | |
knockSlackChannelId: string; | |
tenant: string; | |
} |
tenantId
feels more descriptive to me, but I’m okay with keeping the naming consistent.
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 reason is legacy, we also use tenant
(sans Id
) elsewhere to describe a tenant id, like the data we pass to a workflow trigger, because originally tenants as a concept were a simple string rather than an object under the hood. I don't have a sense for what's less confusing for users - consistency, or more specific naming. I imagine if you're implementing both SlackKit and MsTeams it will be slightly confusing to use different params for the same argument, but I don't feel strongly about keeping it as tenant
because I do think tenantId
is more clear. @cjbell might have a stronger opinion one way or another.
--rtk-error-red: rgba(205, 123, 46, 1); | ||
} | ||
|
||
.rtk-connect__button { |
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 stylesheet is identical to the one we use with SlackAuthButton
, but class names are prefixed with rtk-
(for “React TeamsKit”) instead of rsk-
(for “React SlackKit”).
packages/react-core/src/modules/ms-teams/hooks/useMSTeamsAuth.ts
Outdated
Show resolved
Hide resolved
8db9f4a
to
c661d85
Compare
This PR adds
MsTeamsAuthButton
, the first React component of our new TeamsKit library. Microsoft Teams admins can use this button to initiate the Microsoft OAuth flow. When authorization is successfully granted, the Knock tenant will be associated with the admin user’s Microsoft tenant.The example app shown in the Loom video will be added in a future PR.
Video
https://www.loom.com/share/f09240c1cf5148908d7a49cda4d38977?sid=aee718a2-ce0a-4a6f-a74a-1ba22c28c088