-
Notifications
You must be signed in to change notification settings - Fork 73
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(frontend): add required gates to cli automate page #2948
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.
LGTM! my only comment would be to make it explicit that this is an override. like maybe the form field name could be "Override default required gates".
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.
Hey @jorgeepc I left a couple of questions for some improvements
@@ -82,6 +85,20 @@ const Controls = ({onChange, id, environmentId, fileName, resourceType}: IProps) | |||
</Form.Item> | |||
))} | |||
</S.OptionsContainer> | |||
|
|||
<S.RequiredGatesContainer> |
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.
Would it make sense to have the values be the "pretty" version similar to what we have on the settings page?
We could also have them default to the global values
And finally, something that would help users understand that the required gates are, a tooltip or something
This PR adds the required-gates option to the CLI Automate page.
Changes
Fixes
Checklist
Loom
https://www.loom.com/share/75371b5017cf42b49fe771514cdea079?sid=77d6dab5-1b96-498e-b360-72e4186ce400