-
Notifications
You must be signed in to change notification settings - Fork 377
chore(Text): Convert examples to typescript #8016
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
chore(Text): Convert examples to typescript #8016
Conversation
|
Preview: https://patternfly-react-pr-8016.surge.sh A11y report: https://patternfly-react-pr-8016-a11y.surge.sh |
|
@XiangyuShen nice job on this! I've did a similar pull request some days ago and the reviewers gave me a lot of tips. To best ensure that your first PR here gets merged, I would suggest these few things: • You could rename the TypeScript examples to [componentName][exampleTitle] (eg. • Also, there seems to be an error in the building of the docs: export const TextHeading: React.FunctionComponent = () => (
<TextContent>
<Text component={TextVariants.h1}>Hello World</Text>
<Text component={TextVariants.h2}>Second level</Text>
<Text component={TextVariants.h3}>Third level</Text>
<Text component={TextVariants.h4}>Fourth level</Text>
<Text component={TextVariants.h5}>Fifth level</Text>
<Text component={TextVariants.h6}>Sixth level</Text>
</TextContent>
); |
|
@gefgu Thank you very much for the feedback! Have implemented the changes. My first contribution to open source so thanks for helping. |
thatblindgeye
left a comment
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 looking good! One thing is that all of the other examples should have an export similar to the TextVisited.tsx file, i.e. export const TextBody: React.FunctionComponent... and so on.
|
@thatblindgeye Changes implemented! Thanks. |
thatblindgeye
left a comment
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 making the updates!
| <Text component={TextVariants.h5}>Fifth level</Text> | ||
| <Text component={TextVariants.h6}>Sixth level</Text> | ||
| </TextContent>; | ||
| ```ts file="./TextHeadings.tsx" |
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.
Since the examples are using TextListVariants, I would add it to the documentation so it shows up in the props table. This can be done by adding it to the propComponents array on line 5 of this md file.
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.
TextListVariants does not render in the props table when added. This is likely because it is just an enum in TextList.
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.
@XiangyuShen you are correct! We do not currently have a way to document enums.
tlabaj
left a comment
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! Thanks
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #8000
Convert Text examples to TypeScript
Additional issues: