Skip to content
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

Merged

Conversation

adamviktora
Copy link
Contributor

@adamviktora adamviktora commented Jun 24, 2024

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

@patternfly-build
Copy link
Contributor

patternfly-build commented Jun 24, 2024

@tlabaj tlabaj force-pushed the text-to-content_breaking-change branch from 4ce7667 to 55d6063 Compare July 17, 2024 19:37
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@thatblindgeye thatblindgeye left a 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".

Comment on lines 7 to 9
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>
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor

@edonehoo edonehoo left a 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`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
Copy link
Contributor

@edonehoo edonehoo Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<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 />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 />

@wise-king-sullyman wise-king-sullyman merged commit f067b7c into patternfly:v6 Jul 22, 2024
12 of 13 checks passed
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-code-editor@6.0.0-alpha.88
  • @patternfly/react-core@6.0.0-alpha.88
  • @patternfly/react-docs@7.0.0-alpha.95
  • @patternfly/react-drag-drop@6.0.0-alpha.70
  • @patternfly/react-integration@6.0.0-alpha.46
  • demo-app-ts@5.1.1-alpha.87
  • @patternfly/react-table@6.0.0-alpha.89
  • @patternfly/react-templates@6.0.0-alpha.38

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Text component name to Content to better align with core Text - support for hr element
8 participants