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

feat(clerk-js): Update ui of OrganizationSwitcher to better match OrganizationList #1675

Merged
merged 5 commits into from
Sep 5, 2023

Conversation

panteliselef
Copy link
Member

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

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-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

Description

  • npm test runs as expected.
  • npm run build runs as expected.

This PR implements the following UI

figma

image

commits

image
image
image

@changeset-bot
Copy link

changeset-bot bot commented Sep 4, 2023

🦋 Changeset detected

Latest commit: c38aed5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@clerk/localizations Patch
@clerk/clerk-js Patch
@clerk/types Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/backend Patch
@clerk/fastify Patch
gatsby-plugin-clerk Patch
@clerk/nextjs Patch
@clerk/clerk-react Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch

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

@@ -1,3 +1,5 @@
import type { ClerkPaginatedResponse } from '@clerk/types';
Copy link
Member

Choose a reason for hiding this comment

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

❓ Should we add tests for these utils?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't get this to work with swr, so i decoupled the helpers and tested those instead.

@@ -1,3 +1,5 @@
import type { ClerkPaginatedResponse } from '@clerk/types';
Copy link
Member

Choose a reason for hiding this comment

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

Yes please

...common.unstyledScrollbar(t),
})}
>
{(userInvitations?.count ?? 0) > 0 &&
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a top-level condition to avoid the repeated use of .??

Copy link
Member Author

Choose a reason for hiding this comment

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

userInvitations will always be defined, but count may be undefined so I will change this to

{(userInvitations.count ?? 0) > 0 &&

Do you believe that the default returned value for userInvitations.count should be 0 instead of undefined ?


{(userSuggestions?.count ?? 0) > 0 &&
!userInvitations?.hasNextPage &&
userSuggestions?.data?.map(suggestion => {
Copy link
Member

Choose a reason for hiding this comment

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

Same

@panteliselef panteliselef merged commit 1426e5e into main Sep 5, 2023
5 checks passed
@panteliselef panteliselef deleted the ORG-178 branch September 5, 2023 10:01
@clerk-cookie clerk-cookie mentioned this pull request Sep 5, 2023
@clerk-cookie
Copy link
Collaborator

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.

@clerk clerk locked as resolved and limited conversation to collaborators Sep 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants