-
Notifications
You must be signed in to change notification settings - Fork 353
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
feat(misc): Replace Text, TextContent, TextList and TextListItem with Content - PART 2 #10643
feat(misc): Replace Text, TextContent, TextList and TextListItem with Content - PART 2 #10643
Conversation
Preview: https://patternfly-react-pr-10643.surge.sh A11y report: https://patternfly-react-pr-10643-a11y.surge.sh |
2fb1edc
to
d72cde6
Compare
4ce7667
to
55d6063
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.
LGTM
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.
Few changes below.
Also this isn't a blocker for this PR, but would be worth post v6 to clean up usage of Content for consistency. Right now we're mixing and matching between <Content component="h1">...
and just using a native HTML <h1>...
. Even if both are valid, might be better to stick to one in our examples/codebase, except any examples that are intended to show "you can also use native element inside of Content".
packages/react-core/src/components/Content/examples/ContentPlainList.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Content/examples/ContentPlainList.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Content/examples/ContentOrderedList.tsx
Outdated
Show resolved
Hide resolved
Content with a component of type "p" still renders the same when wrapped with a TextContent. | ||
</Content> | ||
<p>If located within a wrapping TextContent, html elements are styled as well!</p> |
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.
Verbiage may need to be updated here, re: "...TextContent" still being used
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.
@mcoker Is this example even needed. It is a new one?
packages/react-core/src/demos/DescriptionList/examples/DescriptionListBasic.tsx
Outdated
Show resolved
Hide resolved
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 made a bunch of small content change requests too in some of the demos, just that I noticed 😅 but lmk if you have any thoughts or questions about my suggestions!
--- | ||
|
||
The `<Text>` component provides simple, built-in styling for putting common blocks of HTML elements together. It establishes the block of content and styling within it for the elements listed in the `component` property(`h1` through `h6`, `p`, `a`, `small`, `blockquote`, and `pre`), as well as the text component suite `<TextList>`, and `<TextListItem>`. `TextContent` may be used as a container for the text components, but nesting them inside `<TextContent>` is not required. | ||
The `<Content>` component provides simple, built-in styling for putting common blocks of HTML elements together. It establishes the block of content and styling within it for the elements listed in the `component` property(`h1` through `h6`, `hr`, `p`, `a`, `small`, `blockquote`, and `pre`). |
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 `<Content>` component provides simple, built-in styling for putting common blocks of HTML elements together. It establishes the block of content and styling within it for the elements listed in the `component` property(`h1` through `h6`, `hr`, `p`, `a`, `small`, `blockquote`, and `pre`). | |
The `<Content>` component allows you to establish a block of HTML content and apply simple, built-in styling. `<Content>` can be used for any element supported by the `component` property (including `h1` through `h6`, `hr`, `p`, `a`, `small`, `blockquote`, and `pre`). |
|
||
For example, rather than nesting the `<List>` and `<Title>` components within `<Text>`, you should pass `component="h1"` into the `<TextList>` and `<Text>` components. Similarly, when you need to add a divider , rather than passing in a separate `<Divider>` component, you should utilize the `hr` property that `<Text>` supports, which will be styled as a divider. | ||
For example, rather than nesting the `<List>` and `<Title>` components within `<Content>`, you should pass `component="h1"` into the `<Content>` component. Similarly, when you need to add a divider , rather than passing in a separate `<Divider>` component, you should utilize the `hr` property that `<Content>` supports, which will be styled as a divider. |
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.
For example, rather than nesting the `<List>` and `<Title>` components within `<Content>`, you should pass `component="h1"` into the `<Content>` component. Similarly, when you need to add a divider , rather than passing in a separate `<Divider>` component, you should utilize the `hr` property that `<Content>` supports, which will be styled as a divider. | |
For example, to create a level 1 heading, you should pass `component="h1"` to `<Content>`, instead of nesting a `<Title>` component within `<Content>`. Similarly, when you need to add a divider to a page, you should utilize the `hr` property of `<Content>` (which is styled as a divider), rather than using a separate `<Divider>` component. |
maybe I'm misunderstanding, but is mentioning <List>
relevant for this example? It doesn't intrinsically apply h1 does it?
@@ -49,7 +49,7 @@ For example, rather than nesting the `<List>` and `<Title>` components within `< | |||
|
|||
``` | |||
|
|||
Text components such as Text, TextList, TextListItem can be placed within a TextContent to provide styling for html elements, and additional styling options applied to the children. | |||
Html elements wrapped by `<Content>` are styled by the Content component. |
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.
Html elements wrapped by `<Content>` are styled by the Content component. | |
HTML elements wrapped by `<Content>` are styled by the content component. |
Maybe it would make more sense to me if I could load the website preview, but I'm not fully understanding the purpose of this line. Is this different than what we've already stated above?
<Text component="p"> | ||
<Content> | ||
<h1>Main title</h1> | ||
<p> | ||
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> |
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.
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> | |
Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br /> |
this aligns with our higher level typography rules/docs
<Text component="p"> | ||
<Content> | ||
<h1>Main title</h1> | ||
<p> | ||
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> |
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.
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> | |
Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br /> |
<Text component="p"> | ||
<Content> | ||
<h1>Main title</h1> | ||
<p> | ||
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> |
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.
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> | |
Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br /> |
<Text component="p"> | ||
<Content> | ||
<h1>Main title</h1> | ||
<p> | ||
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> |
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.
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> | |
Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br /> |
<Text component="p"> | ||
<Content> | ||
<h1>Main title</h1> | ||
<p> | ||
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> |
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.
Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> | |
Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br /> |
</TextContent> | ||
<Content> | ||
<h1>Projects</h1> | ||
<p>Click any project card to view Tabs within Modals.</p> |
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.
<p>Click any project card to view Tabs within Modals.</p> | |
<p>Click any project card to view tabs within modals.</p> |
<Text component="p"> | ||
<Content> | ||
<h1>Main title</h1> | ||
<p> | ||
Body text should be Overpass Regular at 16px.It should have leading of 24px because <br /> |
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.
Body text should be Overpass Regular at 16px.It should have leading of 24px because <br /> | |
Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br /> |
Needed so that we can avoid temporary build problems caused by a chicken and egg situation between this repo and org.
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #10395
This is a followup to Part 1 PR #10611 - wait for that PR to merge first.
This PR is breaking and the build will fail, because Text, TextContent, TextList and TextListItem have to be updated in
patternfly-org
so the build framework doesn't use these old components.Codemod PR: patternfly/pf-codemods#675
Org issue to replace Text: patternfly/patternfly-org#4087