Skip to content

feat(clerk-js): Use org:sys_domains:read for improved fine grain control of the UI#1896

Merged
panteliselef merged 9 commits into
main-v4from
ORG-286
Oct 31, 2023
Merged

feat(clerk-js): Use org:sys_domains:read for improved fine grain control of the UI#1896
panteliselef merged 9 commits into
main-v4from
ORG-286

Conversation

@panteliselef

@panteliselef panteliselef commented Oct 16, 2023

Copy link
Copy Markdown
Contributor

Description

This PR makes usage of org:sys_domains:read a permission that allows users to see a list of domains. Previously we were wrapping the whole functionality under the manage permission.

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

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

@panteliselef panteliselef self-assigned this Oct 16, 2023
@panteliselef
panteliselef requested a review from a team as a code owner October 16, 2023 21:24
@changeset-bot

changeset-bot Bot commented Oct 16, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9a375d9

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

This PR includes changesets to release 12 packages
Name Type
@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/localizations 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

@panteliselef panteliselef changed the title Org 286 [WIP] Org 286 Oct 17, 2023
@panteliselef panteliselef changed the title [WIP] Org 286 feat(clerk-js): Use org:sys_domains:read for improved fine grain control of the UI Oct 17, 2023
@panteliselef
panteliselef requested a review from anagstef October 17, 2023 16:11
Comment thread packages/clerk-js/src/ui/components/OrganizationProfile/DomainList.tsx Outdated
Comment thread packages/clerk-js/src/ui/components/OrganizationProfile/DomainList.tsx Outdated
},
]}
/>
showDotMenu ? (

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.

🙃 showDotMeny && <JSX/>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Already tried that, seems like trailingComponent does not accept null, I didn't want to mess with the signature of the component.

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.

Why don't we update its types?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The following has type of false | EmotionJSX.Element | null

showDotMenu && (
                    <DomainListDotMenu
                      redirectSubPath={redirectSubPath}
                      domainId={d.id}
                    />
                  )

So i would have to do

(showDotMenu  || null ) && (
                    <DomainListDotMenu
                      redirectSubPath={redirectSubPath}
                      domainId={d.id}
                    />
                  )

Which i'm not convinced it is better

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.

I am not sure I understand what you are talking about. Is this the type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the TS error
image

setData(null);
});
}, []);
}, [JSON.stringify(params)]);

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.

❓ Can you elaborate on this change? Is it necessary to be part of this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Previously useFetch would only fire once, "on mount", there was a bug where the Gate component would not hide the content while not having the necessary permission because the "fetch" wouldn't fire when the parameters changed.

For context we are using useFetch to handle the async logic of isAuthorized.

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.

Do we need to make a FAPI call every time the isAuthorized is invoked?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This would only invoke a call if params change, not on every render.

Comment thread packages/clerk-js/src/ui/utils/test/mockHelpers.ts Outdated
* @returns {string} Returns the string without leading slashes
* @param path
*/
export const trimLeadingSlash = (path: string): string => {

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.

Can we add a couple of tests for this util?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeap, thanks for the reminder

@panteliselef
panteliselef force-pushed the ORG-286 branch 2 times, most recently from a591162 to d19c8a0 Compare October 18, 2023 13:21
Comment thread packages/clerk-js/src/core/resources/Session.ts Outdated
Comment thread packages/clerk-js/src/core/resources/Session.ts Outdated
Comment thread packages/clerk-js/src/ui/components/OrganizationProfile/DomainList.tsx Outdated
},
]}
/>
showDotMenu ? (

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.

Why don't we update its types?

setData(null);
});
}, []);
}, [JSON.stringify(params)]);

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.

Do we need to make a FAPI call every time the isAuthorized is invoked?

Comment thread packages/clerk-js/src/ui/utils/test/mockHelpers.ts Outdated
@panteliselef
panteliselef changed the base branch from main to main-v4 October 25, 2023 18:53
},
]}
/>
showDotMenu ? (

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.

I am not sure I understand what you are talking about. Is this the type?

@panteliselef
panteliselef added this pull request to the merge queue Oct 31, 2023
Merged via the queue into main-v4 with commit f6f67f9 Oct 31, 2023
@panteliselef
panteliselef deleted the ORG-286 branch October 31, 2023 17:05
panteliselef added a commit that referenced this pull request Oct 31, 2023
github-merge-queue Bot pushed a commit that referenced this pull request Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants