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

split permissions server and ui code #611

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

nichtsam
Copy link
Contributor

solves #609

Test Plan

Checklist

  • Tests updated
  • Docs updated

Screenshots

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thank you!

I like the suggestion @gregleeper made. Do you think you could move the permissions.ts contents to user.ts? I think that would make this even better.

@nichtsam nichtsam force-pushed the pr/split-permissions-code branch 2 times, most recently from 0499f71 to 85c3d98 Compare January 29, 2024 20:54
@nichtsam
Copy link
Contributor Author

Definitely! I amended the commit to reflect the suggestion.

However I left PermissionString and parsePermissionString in permissions.ts.
I find this easier to reason about, otherwise it'd be permissions.server.ts importing these thing from user.tsx, and it somehow feels weird to me. Nevertheless, let me know if you would rather have it all moved into user.tsx and I would update again!

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

I think I'm ok moving the parsing utility to user.ts along with the other permission stuff. I don't mind having the permissions.server.ts importing stuff from user.ts :)

Thanks!

@nichtsam nichtsam force-pushed the pr/split-permissions-code branch from 85c3d98 to 04a477e Compare January 30, 2024 09:27
@nichtsam nichtsam requested a review from kentcdodds January 30, 2024 09:29
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Lovely. Thank you!

@kentcdodds kentcdodds merged commit 6eb46d0 into epicweb-dev:main Jan 30, 2024
5 checks passed
@nichtsam nichtsam deleted the pr/split-permissions-code branch January 30, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants