-
Notifications
You must be signed in to change notification settings - Fork 10
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
text input behavior based on IPA object metadata #118
Conversation
912ae33
to
c403fc5
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.
Some minor changes requested. Please see the inline comments.
src/utils/ipaObjectUtils.ts
Outdated
@@ -0,0 +1,155 @@ | |||
import { is } from "cypress/types/bluebird"; |
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.
Typo?
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.
Oversight. (it was added automatically by vsCode). Removed.
@@ -47,6 +57,10 @@ const ActiveUsersTabs = () => { | |||
}, | |||
]; | |||
|
|||
if (metadataLoading) { | |||
return <div>Loading Metadata...</div>; |
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.
Maybe this could be replaced by a Spinner.
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.
Replaced with Spinner on all 3 locations.
user: User, | ||
metadata: Metadata | ||
) { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any |
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.
Hint: You can add the following line at the beginning of the file to disable the TypeScript 'any' warning for the whole file: /* eslint-disable @typescript-eslint/no-explicit-any */
. That prevents you from explicitly adding it line by line.
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 know, but I don't want to disable it for the whole file. I believe there is a value behind this lint check, thus I want to disable it explicitly on the places where I don't see any other choice.
src/components/Form/IpaTextInput.tsx
Outdated
getParamProperties(props); | ||
|
||
if (!writable) { | ||
return <span>{value}</span>; |
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 from now I would keep it as a disabled Text Input instead of a span (as per discussed in the WebUI WG meetings). If we change our minds, this is something easy to change.
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.
Span removed. But I was looking into https://www.patternfly.org/v4/components/text-input/ and noticed that there is: readOnlyVariant
props. Which if set to 'plain' | 'default' makes the input non-modifiable but still more user pleasant than isDisable=true
. Thus I used that as it seems built for this purpose.
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 didn't realize that PF already had a readOnlyVariant
prop. That will be very useful indeed :)
This commit adds loading of metadata and creates an example how a common field logic can be defined in single place (ipaObjectUtils and IpaTextInput) and used for multiple fields - thus limit the amount of code in various sections + make it possible to change behavior/appearance of all fields of the same type on single place. Next steps would be to implement new "Ipa" components for different style of inputs - checkboxes, multi-valued text boxes, ... and use it. Signed-off-by: Petr Vobornik <pvoborni@redhat.com>
c403fc5
to
e053d15
Compare
All review comments are addressed or responded to. |
LGTM. |
This commit adds loading of metadata and creates an example how a common field logic can be defined in single place (ipaObjectUtils and IpaTextInput) and used for multiple fields - thus limit the amount of code in various sections + make it possible to change behavior/appearance of all fields of the same type on single place.
Next steps would be to implement new "Ipa" components for different style of inputs - checkboxes, multi-valued text boxes, ... and use it.