-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
UI: Implement new policy SS + modal designs #17749
Conversation
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 work! I left some comments. It's probably out of the scope of this work but given the complexity of the SearchSelect
component I'm surprised that we have somewhat duplicated it for the modal version. It would be great if we could incorporate the modal into the primary component if possible but I'm not familiar with the component and would love to learn more about it.
@@ -42,6 +42,23 @@ import { dasherize } from 'vault/helpers/dasherize'; | |||
*/ | |||
|
|||
export default class FormFieldComponent extends Component { | |||
// alphabetical list of all editTypes if they hide <FormFieldLabel> | |||
shouldHideLabel = { |
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.
Curious about feedback here. Having this object felt like a nice, concise way to document the list of different edit types and quickly reference whether or not they render a <FormFieldLabel>
.
But if this feels overly complicated I can refactor to more similarly mimic the template's original conditional
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! I wonder if we could iterate on this in the future and export it somewhere and then consume the different types in models as well?
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.
Wowza, great work on this! A few more things to address, although none are major. I'll play around with the branch and then approve if it looks good from there
handleChange() { | ||
if (this.selectedOptions.length && typeof this.selectedOptions.firstObject === 'object') { | ||
if (this.passObject) { |
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 understand we might not need this in the modal version, but at some point we had to be able know in the parent whether the user picked a new item, so that we could show the form inline. Maybe in a future where the two components are merged, we can only show forms in a modal? 🤩
ui/tests/integration/components/search-select-with-modal-test.js
Outdated
Show resolved
Hide resolved
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 work!
@@ -42,6 +42,23 @@ import { dasherize } from 'vault/helpers/dasherize'; | |||
*/ | |||
|
|||
export default class FormFieldComponent extends Component { | |||
// alphabetical list of all editTypes if they hide <FormFieldLabel> | |||
shouldHideLabel = { |
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! I wonder if we could iterate on this in the future and export it somewhere and then consume the different types in models as well?
Includes fixing the wizard
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.
Semgrep Scanner found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
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 beautiful, let's ship it
Really though, great work on this and loved the collaboration involved!
This PR adds the ability to create an ACL or RGP policy inline when creating a group or entity. It also filters out the
root
policy from the dropdown.Example A:
Example B:
Type RGP is disabled unless the user has Sentinel (enterprise feature)