-
Notifications
You must be signed in to change notification settings - Fork 2
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
Issue 50569: Add ability for users to supply descriptions with API keys #1600
Conversation
{type === 'apikey' && !keyValue && ( | ||
<div> | ||
<label htmlFor="keyDescription" className="right-spacing"> | ||
Description (optional) |
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.
we usually indicate the opposite for field labels (i.e. use asterisks for required and nothing for optional). minor but this seems like it could drop the "(optional)" part of the label.
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.
True. I can probably also remove that "right-spacing" class. Left over from previous placement of this element.
type="text" | ||
onChange={changeDescription} | ||
autoFocus | ||
disabled={!!keyValue} |
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 seems unnecessary since we won't be showing the desc input if we have a keyValue
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.
Yup. Left over logic.
{keyValue && <div className="api-key__description">{description}</div>} | ||
{!!keyValue && ( |
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.
minor: is there a reason that one of these checks !!keyValue
and the other doesn't? Seems like they could/should 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.
No good reason. I joined them all under one test for !!keyValue
Rationale
Issue 50569 - Sometimes users generate multiple API keys and want to add a description for when or how each is being used. The server-side addition of these fields has already been done. This PR and its relatives add the client-side changes.
Related Pull Requests
Changes