-
Notifications
You must be signed in to change notification settings - Fork 91
fix(pf4): Add customization api to plain text #697
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
Conversation
@@ -4,11 +4,11 @@ import PropTypes from 'prop-types'; | |||
import { TextContent, Text } from '@patternfly/react-core'; | |||
import validTextFields from '@data-driven-forms/common/src/utils/valid-text-fields'; | |||
|
|||
const PlainText = ({ label, name, variant }) => ( | |||
<TextContent> | |||
const PlainText = ({ label, name, variant, TextProps, TextContentProps }) => ( |
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 TextProps
should be changed to ...rest
to make it consistent with the way we are spreading props in all other components. (...rest
should be always spread to the main component.)
Just catch component
props before you do it.
|
||
export interface PlainTextProps { | ||
label: ReactNode; | ||
name: string; | ||
variant?: 'p'|'span'|'strong'|'b'|'cite'|'caption'|'code'|'em'|'i'|'h1'|'h2'|'h3'|'h4'|'h5'|'h6'|'h6'|'div'|'label'|'pre'|'q'|'samp'|'small'|'sub'|'sup'; | ||
TextProps: TextProps, | ||
TextContentProps: TextContentProps, |
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 we should add it do docs.
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.
Ah right its not generated yet.
Codecov Report
@@ Coverage Diff @@
## master #697 +/- ##
=======================================
Coverage 92.71% 92.71%
=======================================
Files 212 212
Lines 3308 3308
Branches 1074 1074
=======================================
Hits 3067 3067
Misses 241 241
Continue to review full report at Codecov.
|
11fada0
to
5c2ce24
Compare
@@ -4,11 +4,11 @@ import PropTypes from 'prop-types'; | |||
import { TextContent, Text } from '@patternfly/react-core'; | |||
import validTextFields from '@data-driven-forms/common/src/utils/valid-text-fields'; | |||
|
|||
const PlainText = ({ label, name, variant }) => ( | |||
<TextContent> | |||
const PlainText = ({ label, name, variant, TextContentProps, ...rest }) => ( |
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.
please catch 'component' prop coming from renderer, it could cause issues.
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 this actually is closer to the current way of customization in our other mappers (stuff like required, disabled, etc.). Plus the component prop is always caught by the use field hook. And you can't really use the component
key in the schema.
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 plaintext component will receive component="plain_text"
and then this is passed via rest
to the PF Text
component. There is no useFieldApi
hook here to catch it.
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 do not catch component
in useFIeldApi!?
Yup its not interactive field 😆
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 do, but plain text is not using it. It's not a field, it's just a component.
@@ -19,7 +19,8 @@ const PlainText = ({ label, name, variant }) => ( | |||
PlainText.propTypes = { | |||
variant: PropTypes.oneOf(validTextFields), |
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 should mention that component
PF prop is mapped to variant
prop in Docs.
5c2ce24
to
d380c31
Compare
d380c31
to
7d2da4c
Compare
🎉 This PR is included in version 2.8.9 🎉 The release is available on Demo can be found here! |
No description provided.