-
Notifications
You must be signed in to change notification settings - Fork 415
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
feat: Add manage segment overrides permission in UI #2936
feat: Add manage segment overrides permission in UI #2936
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
vercel is not working for me to review this. Maybe @kyle-ssg is a better person to review this? |
<Permission | ||
level='project' | ||
permission={'MANAGE_SEGMENTS'} | ||
id={this.props.projectId} | ||
> |
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.
Shouldn't this permission be MANAGE_SEGMENT_OVERRIDES
?
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 was following the advice you left in this comment. #1676 (comment)
In addition to the above, please also hide the Create Feature-Specific Segment button if the user is missing the project permission MANAGE_SEGMENTS since they will not be able to create a segment if that permission is missing.
I thought it made sense.
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 in case, this button creates a Feature-Specific segment.
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.
Ahh, of course. Thanks for double checking Novak!
Using this branch |
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.
The code overall looks good, but I have a couple of small comments / questions.
if (this.state.selectedSegment) { | ||
this.props.setShowCreateSegment(false) | ||
} else { | ||
this.props.setShowCreateSegment(false) |
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 think you can remove the if-else statement here and stick to just one if statement, since both branches of the if-else statement call the same thing.
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.
Good catch, this is not my code, it is a change in the format, but we can optimize it. Done.
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.
Looks good overall!
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingChanges
How did you test this code?