-
Notifications
You must be signed in to change notification settings - Fork 35
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
✨ Config based UI #123
✨ Config based UI #123
Conversation
…r-subject-support
…zone-ui into config-page
Your Render PR Server URL is https://ozone-staging-pr-123.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cp76h10l6cac7380efeg. |
Your Render PR Server URL is https://ozone-sandbox-pr-123.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cp76h1ol6cac7380efkg. |
lib/client.ts
Outdated
throw new Error( | ||
"Account does not have access to this Ozone service. If this seems in error, check Ozone's access configuration.", | ||
) | ||
throwUnAuthorizedError() |
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 definitely a nit, but I think it's nice to keep control flow transparent— throw
ing is already a part of the language that can be a little difficult to reason about (at least for me!) 😆
The suggestion here would be to reorganize it so that it's something like:
throwUnAuthorizedError() | |
throw createUnauthorizedError() |
One upside of doing things this way is that typescript can tell when code is unreachable after a throw
or certain if
conditions are guaranteed to have been met (similar to its treatment of return
). When the throw
is behind a function it's a bit less transparent. (The syntax highlighting around control flow statements is also a win!) It's not actually a real issue here, in my view it's just a nice standard practice to lean that works well in trickier cases.
canManageTemplates: isModerator, | ||
canTakedown: !!config.pds?.url && isModerator, | ||
canLabel: isModerator, | ||
canManageChat: !!config.chat?.url && isModerator, | ||
canSendEmail: !!config.pds?.url && isModerator, |
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 double-checking that there aren't any admin-only actions.
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 are a couple of admin only things like managing team members (which we just merged) and feedgen takedown and I just added those 2 in.
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.
Lovely
Use the config published from ozone server here to only show relevant content based on viewer's role.
Also, display the config in the configuration page.