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

Add support for classes on TextContent children #10262

Closed
mcoker opened this issue Apr 9, 2024 · 1 comment · Fixed by #10378
Closed

Add support for classes on TextContent children #10262

mcoker opened this issue Apr 9, 2024 · 1 comment · Fixed by #10378
Assignees
Labels
PF6 Tech debt improvements to code that do not affect either user or product developers’ experience.

Comments

@mcoker
Copy link
Contributor

mcoker commented Apr 9, 2024

follow up for core issue patternfly/patternfly#6511

A quick overview of the change:

The current <TextContent> and <Text> component(s) basically work this way:

  • <TextContent> serves as a wrapper and styles/formats simple HTML elements within it.
  • <Text> renders simple HTML elements.
  • Exhibit A. Browser rendered content -> react code -> simple HTML elements rendered in the content wrapper in dev tools
    • Screenshot 2024-04-09 at 10 50 44 AM

This is cool, but comes with some issues.

  • Since the markup has to be in the <TextContent> wrapper, you can't really sprinkle use of formatted content around the page. Doing so would require the <TextContent> wrapper for everywhere it's used, and you don't get the intended spacing/margin unless all of the things you want to have space/margin are in the <TextContent> wrapper, and not everything plays nicely in the wrapper, which is the next point.
  • Since <TextContent> styles its children based off of their element type (<p>, <ul>, <h1>, etc), if you put another component in it (eg, <List>), <TextContent> will apply its styling to any of the elements it supports in the component you've included, which 90% of the time will probably break the component styling.

The update in core leaves the existing basic styling as it is. Elements are still styled via HTML elements as children of <TextContent> - .pf-v6-c-content p, .pf-v6-c-content h1, etc. But it extends all of the existing styles to also apply to a class for the child element used (.pf-v6-c-content--p, .pf-v6-c-content--h1, etc) and that class can be used inside or outside of <TextContent>.

My proposal would be to:

  • Update <Text>, <TextList>, and <TextItem> to simply include the new class appropriate for the element
    • <Text component={TextVariants.p}> renders <p class="pf-v6-c-content--p">
  • Make <TextContent> optional in the docs
  • Clarify in the docs that <Text>, <TextList>, and <TextItem> can be used on their own outside of <TextContent>, and if there is styling for something like :is(.pf-v6-c-content p, .pf-v6-c-content--p):not(:last-child) { // a bottom margin }, a paragraph will now have a bottom margin if it comes before anything else (like a <Table> for example)
  • Add a new example with no <TextContent> wrapper. Or update all of the examples to remove the <TextContent> wrapper, and include a simple example that uses <TextContent> - I think would be my preference, I imagine this is the most common use case.
@mcoker mcoker added the Tech debt improvements to code that do not affect either user or product developers’ experience. label Apr 9, 2024
@tlabaj tlabaj added the PF6 label Apr 10, 2024
@tlabaj tlabaj added this to the Penta beta release milestone Apr 10, 2024
@mcoker mcoker self-assigned this Apr 16, 2024
@kmcfaul kmcfaul assigned kmcfaul and unassigned mcoker Apr 23, 2024
@kmcfaul
Copy link
Contributor

kmcfaul commented May 7, 2024

Depends on #10314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PF6 Tech debt improvements to code that do not affect either user or product developers’ experience.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants