Construct urls based on Organization or User <OrganizationSwitcher/> #1503
Conversation
panteliselef
requested review from
SokratisVidros and
nikosdouvlis
as code owners
🦋 Changeset detectedLatest commit: 7017b8e The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
panteliselef
force-pushed
the
ORG-64
branch
2 times, most recently
from
440127a
to
043c566
Compare
|
!snapshot |
|
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: use the snippet copy button below to quickly install the required packages. # @clerk/backend
npm i @clerk/backend@0.26.1-snapshot.043c566# @clerk/chrome-extension
npm i @clerk/chrome-extension@0.3.25-snapshot.043c566# @clerk/clerk-js
npm i @clerk/clerk-js@4.54.2-snapshot.043c566# eslint-config-custom
npm i eslint-config-custom@0.3.0# @clerk/clerk-expo
npm i @clerk/clerk-expo@0.18.16-snapshot.043c566# @clerk/fastify
npm i @clerk/fastify@0.6.3-snapshot.043c566# gatsby-plugin-clerk
npm i gatsby-plugin-clerk@4.4.3-snapshot.043c566# @clerk/localizations
npm i @clerk/localizations@1.23.3-snapshot.043c566# @clerk/nextjs
npm i @clerk/nextjs@4.23.1-snapshot.043c566# @clerk/clerk-react
npm i @clerk/clerk-react@4.23.2-snapshot.043c566# @clerk/remix
npm i @clerk/remix@2.9.1-snapshot.043c566# @clerk/clerk-sdk-node
npm i @clerk/clerk-sdk-node@4.12.1-snapshot.043c566# @clerk/shared
npm i @clerk/shared@0.20.0# @clerk/themes
npm i @clerk/themes@1.7.5# @clerk/types
npm i @clerk/types@3.48.2-snapshot.043c566 |
panteliselef
changed the title
|
!snapshot |
|
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: use the snippet copy button below to quickly install the required packages. # @clerk/backend
npm i @clerk/backend@0.28.0-snapshot.a1792a4# @clerk/chrome-extension
npm i @clerk/chrome-extension@0.3.27-snapshot.a1792a4# @clerk/clerk-js
npm i @clerk/clerk-js@4.56.0-snapshot.a1792a4# eslint-config-custom
npm i eslint-config-custom@0.3.0# @clerk/clerk-expo
npm i @clerk/clerk-expo@0.18.18-snapshot.a1792a4# @clerk/fastify
npm i @clerk/fastify@0.6.4-snapshot.a1792a4# gatsby-plugin-clerk
npm i gatsby-plugin-clerk@4.4.5-snapshot.a1792a4# @clerk/localizations
npm i @clerk/localizations@1.24.2-snapshot.a1792a4# @clerk/nextjs
npm i @clerk/nextjs@4.23.3-snapshot.a1792a4# @clerk/clerk-react
npm i @clerk/clerk-react@4.23.3-snapshot.a1792a4# @clerk/remix
npm i @clerk/remix@2.9.2-snapshot.a1792a4# @clerk/clerk-sdk-node
npm i @clerk/clerk-sdk-node@4.12.3-snapshot.a1792a4# @clerk/shared
npm i @clerk/shared@0.21.0# @clerk/themes
npm i @clerk/themes@1.7.5# @clerk/types
npm i @clerk/types@3.50.0-snapshot.a1792a4 |
packages/clerk-js/src/ui/components/OrganizationSwitcher/OrganizationSwitcherPopover.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/OrganizationSwitcher/OrganizationSwitcherPopover.tsx
Outdated
Show resolved
Hide resolved
| return urlWithParam; | ||
| }; | ||
|
|
||
| const parse = createDynamicParamParser({ regex: /:(\w+)/ }); |
There was a problem hiding this comment.
❓ Do we have a need for multiple parsers right now? If not, I'd prefer a more explicit/simple approach with a more explicit name for the parse function.
There was a problem hiding this comment.
I do like having the createDynamicParamParser util here. I renamed it to populateParamFromObject
There was a problem hiding this comment.
Okay! Can we move it to its own file and write a couple of unit tests covering it? I see the tests for ClerkUIComponentsContext but lets include a few tests to better document this one :)
| const navigateAfterSwitchOrganization = () => | ||
| ctx.afterSwitchOrganizationUrl ? navigate(ctx.afterSwitchOrganizationUrl) : Promise.resolve(); | ||
|
|
||
| const navigateAfterSwitchOrganization = ({ |
There was a problem hiding this comment.
Are we going to keep the internal name or should we rename this to navigateAfterSelectOrganization?
There was a problem hiding this comment.
Not sure if that would be helpful.
If we split this to 2 function then we are exposing the conditional logic inside of handlePersonalWorkspaceClicked in order to have the same result. If in any case we want to create more than 2 handlePersonalWorkspaceClicked functions in more than 2 components this will result to more duplicate code.
There was a problem hiding this comment.
Sorry If I wasn't clear - I'm just saying that we probably want to rename this to better match the new public API
There was a problem hiding this comment.
Good job @panteliselef :)
Added an extra comment but approved!
…rganizationSwitcher/> + Introduces dynamicParamParser (+tests) + Add types for functions and string literals in OrganizationSwitcherProps & CreateOrganizationProps + afterSelectPersonalUrl & afterSelectOrganizationUrl
|
This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Type of change
Packages affected
@clerk/clerk-js@clerk/clerk-react@clerk/nextjs@clerk/remix@clerk/types@clerk/themes@clerk/localizations@clerk/clerk-expo@clerk/backend@clerk/clerk-sdk-node@clerk/shared@clerk/fastify@clerk/chrome-extensiongatsby-plugin-clerkbuild/tooling/choreDescription
npm testruns as expected.npm run buildruns as expected.Demo for this PR
You can test the changes of this PR in this app