-
Notifications
You must be signed in to change notification settings - Fork 329
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.
The video and code looks good so far! Left a couple of suggestions.
You'll need to address the failing tests and add a changelog too.
@@ -150,6 +150,9 @@ form: | |||
variable_name_placeholder: 'var_key' | |||
variable_value: 'Value' | |||
variable_value_placeholder: 'var_value' | |||
variable_set_sensitive: Set as sensitive | |||
variable_value_placeholder_sensitive: 'Sensitive - write only' | |||
variable_form_placeholder_sensitive: 'sensitive - write only' |
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.
(suggestion) Have the casing for both be 'Sensitive - write only' so that they match?
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.
This is the way it is in HCP and the accompanying designs so I'm just making them match.
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.
Makes sense, but we should follow https://design-system-components-hashicorp.vercel.app/content/writing-guidelines in the future to be consistent with casing
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.
@joshklekamp or @ryenotbread what do you think? This difference in case is how it works in TFC which is what I modeled ours on.
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 they should match and they should be sentence cased.
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 agree they should match and be sentence cased.
Co-authored-by: Jamie White <[email protected]>
73b7ac6
to
2afaa79
Compare
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!
Seems like there were some non-quote auto-prettier edits, but not super important
Closes #2138
Purpose of change
This PR adds a toggle switch on the input variables create/edit form for making an input "sensitive". If the input is set as such, in the list, there will be a badge in place of the displayed which says
Sensitive - write only
. If the user edits the variable, the value will be hidden and only the placeholder textsensitive - write only
will be visible in the text input. The user will be unable to toggle off the sensitive switch and see the variable. If they toggle sensitive off, they will need to provide a new value in the input. This behavior matches HCP. Please note this does not obscure the value from network requests.Two component tests have been added.
Demo
Screen.Recording.2022-10-31.at.3.40.37.PM.mov
To test the change