-
Notifications
You must be signed in to change notification settings - Fork 11
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
[Active users][Settings][Account settings] Add functionality for text inputs with buttons #140
[Active users][Settings][Account settings] Add functionality for text inputs with buttons #140
Conversation
fe5f100
to
63249bd
Compare
16e24fb
to
c5e5c62
Compare
c5e5c62
to
28ad7ab
Compare
|
||
const deletionConfModalActions = [ | ||
<Button | ||
key="add-principal-alias" |
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.
Should this key name be something like: "del-principal-alias", just making sure its not a copy/paste issue
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! You are right, I copy-pasted that and forgot to change the name. Fixing that...
const textInputModalActions = [ | ||
<SecondaryButton | ||
key="add-principal-alias" | ||
onClickHandler={onAddPrincipalAlias} |
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.
Is there anyway to disable this button if the "Add" value is empty?
isDisabled={add_value === "" : true ? 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.
Actually I think you can do it like this using the prop newValue:
isDisabled={newValue === "" : true ? false}
onChange={props.setNewValue} | ||
type="text" | ||
aria-label={props.textInputName} | ||
isRequired={true} |
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.
Not sure if it applies in all cases but when the value is empty you can make the input field "invalidated":
validated={props.newValue === "" ? ValidatedOptions.error : ValidatedOptions.default}
Need to import PF component: ValidatedOptions
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.
That's a good idea. I went further and included another validation check: the validation of the value should be correct only if this is not empty AND if the given realm (if specified) matches with the current one in the system.
I have done that by defining the getRealmFromKrbPolicy
function (utils.ts
) that retrieves the current realm from the krbtpolicy
value in the metadata. The logic is handled from the PrincipalAliasMultiTextboxComponent
as follows:
// Current realm
const currentRealm = getRealmFromKrbPolicy(props.metadata); // <--- Already defined function in 'utils.ts'
// Get realm from 'newAliasValue' (if any)
const getRealmNewAliasValue = () => {
const realm = newAliasValue.split("@")[1];
return realm;
};
const newAliasValueRealm = getRealmNewAliasValue();
// Validation for textInput: realm matches with the current realm (if specified)
const areRealmsMatching =
newAliasValue !== "" &&
newAliasValue.includes("@") &&
currentRealm !== "" &&
newAliasValueRealm !== undefined &&
newAliasValueRealm === currentRealm;
(...)
return (
(...)
<AddTextInputFromListModal
newValue={newAliasValue}
setNewValue={setNewAliasValue}
title={"Add kerberos principal alias"}
isOpen={isTextInputModalOpen}
onClose={onCloseTextInputModal}
actions={textInputModalActions}
textInputTitle={"New kerberos principal alias"}
textInputName="krbprincalname"
textInputValidator={areRealmsMatching} // <-- Passing the validation via props
/>
);
The functionality to take data directly from the metadata needs to be implemented for the Text inputs with buttons (list of text inputs). This should be applied to the following field: 'Principal alias' (`krbprincipalname`). Signed-off-by: Carla Martinez <[email protected]>
28ad7ab
to
c9e299f
Compare
@mreynolds389 - Thanks for the review. I have already updated the code based on your comments. |
onClickHandler={onAddPrincipalAlias} | ||
// isDisabled={newAliasValue === "" ? true : false} | ||
isDisabled={ | ||
(newAliasValue !== "" && !newAliasValue.includes("@")) || |
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.
Sorry didn't test this yet, but isn't this backwards? Shouldn't it be: newAliasValue === "" || !newAliasValue.includes("@") || ...
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.
Not really. This line is trying to test if any of the following cases is happening (one OR the other):
- The inserted text (
newAliasValue
) is not empty and this text doesn't include the '@' symbol --> Enable the 'Add' button - The inserted text is not empty, has the '@' symbol, and what it follows to that symbol matches with the current realm --> Enable the 'Add' button
I have tried the same code on my local instance and it seems to work fine.
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.
Thanks for confirming!
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
The functionality to take data directly from the metadata has been implemented for the Text inputs with buttons (list of text inputs retrieved from the API call). This has been applied to the following field: 'Principal alias' (
krbprincipalname
).