Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DX Guide: Using Organization Slugs in URLs #1664
DX Guide: Using Organization Slugs in URLs #1664
Changes from 4 commits
91a05e1
3d68044
edf0892
6ffcb5d
a1bf396
a51a8e2
e56fb54
a481920
90a4442
6154d63
131e37f
30139c3
4c4769e
06c736d
f7bb4b5
50e2dbc
9b2d01e
4400a26
44b5482
d3b05e6
88d8811
d5c6100
ef0c553
787772b
c924ca3
7da5194
6e3000b
fe23e75
2826a92
1b5b76f
5d7de1a
2906661
d8376f2
11be60c
c69d69d
5d07c2e
e16822a
e438200
0425c13
4832100
d9bf42f
abf0670
7790bdb
1fafbe2
f65327b
3957f81
819716f
ba9a40b
42f5802
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does this feel clear enough to folks? If not, I can add an example. It would be something like:
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.
Instead of "what should be displayed", how about more along the lines of "what should be returned" by the
auth
helperThere 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.
Can you elaborate a bit here? What are examples of what would be returned by the
auth
helper?With this section, i'm attempting to define the application's requirements in a fairly clerk-agnostic way, so that I can then show how it can be modeled using clerk.
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 meant what's returned as the
orgSlug
fromauth
. This doesn't need to be used within a server component to be displayed, it could also be used within a server action.But I understand your goal here and it also looks good to me since this guide is mostly components-based, feel free to go as you prefer.
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.
By "sample application" - are we referencing https://github.com/clerk/orgs/tree/main/examples/sync-org-with-url?
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.
Kind of! What I mean is something like:
The hypothetical application that we'll be fleshing out in this guide (which can also be examined in full at https://github.com/clerk/orgs/tree/main/examples/sync-org-with-url) uses the personal account.
Do you think that a bare
This
is clear enough?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.
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's more like "if your app uses organization IDs instead of slugs in URLs, you'll need to use :id instead if :slug to for navigation to work". I'll workshop another less-personal version.
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.
how about 9b2d01e ?
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.
IMO those "caution" blocks can be a bit alarming to developers. I'd suggest removing this block and putting more specific guidance with comments on the code block below.
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.
Agree with Laura. We should avoid having multiple callouts especially ones that are lengthy.
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'll be honest, inspiring mild alarm in the reader is what I was going for. If a developer were to just look at the organization slug in the URL, assume this meant that the middleware authorized access to the org, then render some sensitive org-specific content from their backend, that would be a very bad bug.
Hey @BRKalow , do you happen to have an opinion on the best way to communicate this one?