-
Notifications
You must be signed in to change notification settings - Fork 951
chore: Updates docs from Protect -> Show #2916
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
base: core-3
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
997960e to
6f8c6b7
Compare
cf2b731 to
fe3906b
Compare
|
how can I test this? is there a snapshot or something? 👀 I need to test for all the SDKs @jacekradko @alexcarpenter |
|
@alexisintech the dashboard is using the updated APIs if you have that pulled down and running. or install |
brkalow
left a comment
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.
Documentation Review
I reviewed this PR against the source code changes in clerk/javascript@f1f1d09. Found a few issues noted in line comments below.
Summary
| Issue | Severity | Lines |
|---|---|---|
Wrong plan instead of feature in Feature examples |
Critical | 850-858, 873-882 |
Missing 'signed-out' in Properties type |
Medium | 1117 |
Inconsistent prop patterns (shorthand vs when) |
Needs Clarification | Multiple |
| <Protect | ||
| permission="premium_access:manage" | ||
| <Show | ||
| when={{ permission: 'premium_access:manage' }} |
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.
Should all of these follow has syntax?
Meaning, should all of these be org:premium_access:manage instead?
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.
i believe i remember that you keep or omit org: prefix
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.
just tested this and yes, you can omit the org: prefix - however, we can be consistent across the docs and add it, it doesn't hurt.
docs/guides/billing/for-b2b.mdx
Outdated
| You can use Clerk's Features, Plans, and Permissions to gate access to content using [authorization checks](!authorization-check). There are a few ways to do this, but the recommended approach is to either use the [`has()`](/docs/reference/backend/types/auth-object#has) method or the [`<Show>`](/docs/reference/components/control/show) component. | ||
|
|
||
| The `has()` method is available for any JavaScript-based framework, while `<Protect>` is a component, and therefore, is only available for React-based frameworks. | ||
| The `has()` method is available for any JavaScript-based framework, while `<Show>` is a component, and therefore, is only available for React-based frameworks. |
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.
while
<Show>is a component, and therefore, is only available for React-based frameworks.
Is this true since we have examples for Astro, Vue, and Nuxt?
If it needs to be reworded, its in docs/guides/billing/for-b2c.mdx too.
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.
nice catch!! updated: b8aa71a
| ```astro {{ filename: 'src/pages/protected-premium-content.astro' }} | ||
| --- | ||
| import { Protect } from '@clerk/astro/react' | ||
| import { Show } from '@clerk/astro/react' |
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.
I’m a novice at Astro, so I could be completely wrong here.
But should these be importing @clerk/astro/components instead and using slot="fallback" instead?
See here → https://clerk.com/docs/astro/reference/components/control/protect
If true, we’ll want to update these too:
- docs/_partials/billing/checking-plan-using-show.mdx
- docs/_partials/billing/checking-permission-using-show.mdx
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.
it will be from @clerk/astro/components if the user isn't using react. if they try to use @clerk/astro/react, they'll get an error, because they haven't configured react.
if they've configured their astro app to use react, they can use @clerk/astro/react.
we should be using the @clerk/astro/components import as we usually assume the user followed the clerk astro quickstart guide, which does not configure react
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.
updated to use @clerk/astro/components as the import b8aa71a
| 1. Create the `dashboard/` directory. | ||
| 1. In the `dashboard/` directory, create a `_layout.tsx` file with the following code. The [`useAuth()`](/docs/reference/hooks/use-auth) hook is used to access the user's authentication state. If the user is already signed in, they'll be redirected to the home page. The [`<Protect>`](/docs/reference/components/control/protect) component is used to ensure that only users with the `org:dashboard:access` Permission can access it. You can modify the `permission` attribute to fit your use case. | ||
| 1. In the `dashboard/` directory, create a `_layout.tsx` file with the following code. The [`useAuth()`](/docs/reference/hooks/use-auth) hook is used to access the user's authentication state. If the user is already signed in, they'll be redirected to the home page. The [`<Show>`](/docs/reference/components/control/show) component is used to ensure that only users with the `org:dashboard:access` Permission can access it. You can modify the `permission` attribute to fit your use case. |
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.
You can modify the
permissionattribute to fit your use case.
Should this say when={{ permission: ... }} instead?
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.
oo good catch - i looked for other instances of this and fixed those as well: 01d4e46
| title: '`<Protect>`' | ||
| description: The Protect component protects content or even entire routes based on authentication, and optionally, authorization. It only renders its children when the current user is signed-in, and if performing authorization checks, if the user has been granted a specific type of access control (Role, Permission, Feature, or Plan). | ||
| title: '`<Show>`' | ||
| description: The Show component protects content or even entire routes based on authentication, and optionally, authorization. It only renders its children when the current user is signed-in, and if performing authorization checks, if the user has been granted a specific type of access control (Role, Permission, Feature, or Plan). |
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.
It only renders its children when the current user is signed-in
I think this needs to be reworded. According to clerk/javascript@f1f1d09, <Show when="signed-out"> appears to be supported.
If true, other coding examples below may need to be updated too.
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.
nice catch! i've just removed that sentence and simplified the description: c1a222c
- Fix react-router and tanstack Feature examples using plan instead of feature - Add 'signed-out' to when prop type definition - Standardize Astro/Vue/Nuxt examples to use when prop instead of shorthand - Replace condition prop with when for callback function examples
ab03639 to
ddbd113
Compare
|
@manovotny thanks for the eyes on this!! i haven't approved it yet because i still have to test all of the code - my review was a basic overview. once i test the functionality with all the sdk's i'll ensure all these examples are working, and also address the feedback you've left (great catches!) |
|
alrighty, tested all sdk's and they're looking great except for the astro bug i posted, and then just need to test expo - which is being worked on by chris right now! |
|
@alexisintech Can you try this syntax for the astro fallback: |

🔎 Previews:
What does this solve?
What changed?