-
Notifications
You must be signed in to change notification settings - Fork 376
feat(Hero): added component #12131
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(Hero): added component #12131
Conversation
|
Preview: https://pf-react-pr-12131.surge.sh A11y report: https://pf-react-pr-12131-a11y.surge.sh |
| @@ -0,0 +1,19 @@ | |||
| --- | |||
| id: Hero | |||
| section: components | |||
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.
Can this be placed under the Patternfly AI / Generative UI section for now instead of Components?
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.
@nicolethoen would we want it to live there in Core as well (assuming that will require a little lift since that section doesn't render in Core docs build currently)?
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.
Wouldn't this living in Components make more sense if the intent here is to abstract it to be something that is usable in non-compass layouts?
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'm team Components personally
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.
feel free to tweak! I'm not super familiar with the component yet, so may have misspoken
| ## Examples | ||
|
|
||
| ### Basic | ||
|
|
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.
| A hero can be used to welcome users to a page, with flexibility to support background images for added visual emphasis. |
Winging this - I'm sure you could say this more accurately - but could we add some sort of brief context for what the component is? I suppose this could also be on the design guidelines tab if we plan to add one, but should work just fine here for now either way
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 yeah was going to say/ask whether this would more be something that appears at the top of the component page like other components:
That could live in Org, which I imagine I may end up putting a PR up there to tweak some Compass demos that live there anyway. Would you still wwant some verbiage here for now?
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.
oh yes, exactly! I was looking at other components pages via this pr's link and was getting so confused that I didn't see those descriptions. But I'm now realizing that's just because they live in org (I assume). Feel free to leave off this sentence here, then!
|
|
||
| ### Basic | ||
|
|
||
| When using a `<Hero>` with a background image, you can use the `<HeroBody>` to prevent text content from overlapping the background image, which helps ensure the text is more easily readable. Keep in mind that you should still check the color contrast when using the `gradientLight` or `gradientDark` properties. |
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.
| When using a `<Hero>` with a background image, you can use the `<HeroBody>` to prevent text content from overlapping the background image, which helps ensure the text is more easily readable. Keep in mind that you should still check the color contrast when using the `gradientLight` or `gradientDark` properties. | |
| To ensure readability and prevent text from overlapping with the background image, use the `<HeroBody>` component. The `<HeroBody>` is optional and can be omitted if there's no background image or if you'd like finer control over text placement. | |
| When using `gradientLight` or `gradientDark`, regardless of the presence of a `<HeroBody>`, check the color contrast to ensure proper accessibility. |
|
|
||
| When using a `<Hero>` with a background image, you can use the `<HeroBody>` to prevent text content from overlapping the background image, which helps ensure the text is more easily readable. Keep in mind that you should still check the color contrast when using the `gradientLight` or `gradientDark` properties. | ||
|
|
||
| You can omit the `<HeroBody>` for finer control over the text placement, or if no background image is applied to the `<Hero>`. |
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.
| You can omit the `<HeroBody>` for finer control over the text placement, or if no background image is applied to the `<Hero>`. |
I brought it up in my previous comment
| import heroGradientStop2Dark from '@patternfly/react-tokens/dist/esm/c_hero_gradient_stop_2_dark'; | ||
| import heroGradientStop3Dark from '@patternfly/react-tokens/dist/esm/c_hero_gradient_stop_3_dark'; | ||
|
|
||
| /** The main Hero component that allowws adjusting of its background images and gradients in different color modes (such as light and dark). */ |
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 main Hero component that allowws adjusting of its background images and gradients in different color modes (such as light and dark). */ | |
| /** The main Hero component that allows adjusting of its background images and gradients in different color modes (such as light and dark). */ |
just noticed a small typo
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 really need to fix my w key or get a new keyboard 😭
| import styles from '@patternfly/react-styles/css/components/Hero/hero'; | ||
| import { css } from '@patternfly/react-styles'; | ||
|
|
||
| /** An optional wrapper component for the body content of a Hero that can be used to help control any text content from overlapping a Hero background image. */ | ||
|
|
||
| export interface HeroBodyProps extends Omit<React.HTMLProps<HTMLDivElement>, 'content'> { | ||
| /** Content of the hero */ | ||
| children?: React.ReactNode; | ||
| /** Additional classes added to the hero */ | ||
| className?: string; | ||
| } | ||
|
|
||
| export const HeroBody: React.FunctionComponent<HeroBodyProps> = ({ className, children, ...props }) => ( | ||
| <div className={css(styles.heroBody, className)} {...props}> | ||
| {children} | ||
| </div> | ||
| ); | ||
|
|
||
| HeroBody.displayName = 'HeroBody'; |
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 wonder if it really makes sense for this to be a standalone component instead of just part of Hero.
In what scenarios would a consumer need to omit this 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.
Hero supports a background image that by default is on the right side of the box. HeroBody is just a div that supports customizations for width and max-width to keep the text from extending over the background image.
If you didn't provide a background image and wanted your text to span the width of the hero box, you could just omit it. We have a similar model in other components. Usually what a [Component]Body element does is provide padding/spacing, and if you omit it, whatever you put in the component spans the full width/height.
width and maxWidth should probably be props that set --pf-v6-c-hero__body--Width and --pf-v6-c-hero__body--MaxWidth though.
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.
If the width and maxWidth are customizable could a consumer not using a background image not just set them to 100%?
a968162 to
213f6be
Compare
| The `bodyWidth` and `bodyMaxWidth` properties can be used for finer control over the placement of text content within the `<Hero>`, such as if you omit a background image. Be mindful of adjusting these properties when a background image is still present, as you may need to ensure the contrast of text is sufficient should it overlap the image. | ||
|
|
||
| When using `gradientLight` or `gradientDark`, check the color contrast to ensure proper accessibility. |
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.
@edonehoo with an update that removed the HeroBody component (we're just baking it into Hero now), I changed the example verbiage a bit, could you give another quick look?
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.
yep! just left a suggestion. Debating on if we should say more about the gradient props or not - like how people should define the stops and colors? unless our plan is to ship pre-made gradient options
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 don't think we plan on shipping any gradients or images for the time being. We can ask that in tomorrow's working session though.
mcoker
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.
Looks like the demo is failing because it's still importing HeroBody
| CompassPanel, | ||
| CompassMessageBar, | ||
| Hero, | ||
| HeroBody, |
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.
| HeroBody, |
| <div>Hero</div> | ||
| <CompassHero> | ||
| <Hero gradientDark={{ stop1: '#000', stop2: '#1b0d33', stop3: '#3d2785' }}> | ||
| <HeroBody>Hero</HeroBody> |
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.
| <HeroBody>Hero</HeroBody> | |
| Hero |
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 after the two minor references to the removed HeroBody component that Michael already pointed out are cleaned up, and one verification question I had re: the custom styles for hero body.
| return ( | ||
| <div | ||
| className={css(styles.hero, hasNoGlass && styles.modifiers.noGlass, className)} | ||
| style={{ ...props.style, ...customStyles }} |
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.
cc @mcoker Do we need to set the two hero body widths on the hero body element or is it fine to set them all on the hero element? Just wanted to check.
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.
Either way. I would probably put them on the hero wrapper since that's where we suggest consumers put their customizations*.
* To expand on that, this is the hero component CSS var block. It renders this in the CSS:
The way we want consumers to customize components is the same way. In their CSS that loads after patternfly CSS (after so that it overrides ours), they should write something like:
.pf-v6-c-hero {
--pf-v6-c-hero--BackgroundColor: lemonchiffon;
--pf-v6-c-hero--BorderStyle: dotted;
--pf-v6-c-hero__body--MaxWidth: 42 parsecs;
... and so on
}
So when we have a component prop that customizes some CSS var for that component, we're effectively just doing that customization for them, so you'd do it the same way.
You don't have to though if it's difficult - like a nested thing where it isn't easy to pass a style object back up to a parent wrapper. Another good use case for adding the vars directly to a hero body would be - if we went back to having a HeroBody component and we supported multiple HeroBodys inside of Hero, then we/users would set the --Width/MaxWidth vars on each HeroBody to customize them individually. If you wanted them all to be the same, you'd put the customization at the component wrapper level.
| The `bodyWidth` and `bodyMaxWidth` properties can be used for finer control over the placement of text content within the `<Hero>`, such as if you omit a background image. Be mindful of adjusting these properties when a background image is still present, as you may need to ensure the contrast of text is sufficient should it overlap the image. | ||
|
|
||
| When using `gradientLight` or `gradientDark`, check the color contrast to ensure proper accessibility. |
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 `bodyWidth` and `bodyMaxWidth` properties can be used for finer control over the placement of text content within the `<Hero>`, such as if you omit a background image. Be mindful of adjusting these properties when a background image is still present, as you may need to ensure the contrast of text is sufficient should it overlap the image. | |
| When using `gradientLight` or `gradientDark`, check the color contrast to ensure proper accessibility. | |
| If you need finer control over the placement of text content within the `<Hero>`, such as when you omit a background image, adjust the `bodyWidth` and `bodyMaxWidth` properties. Be mindful of using these properties when a background image is still present and ensure there is sufficient contrast between text and any part of the image that it overlaps. | |
| When using `gradientLight` or `gradientDark` to apply gradient backgrounds, check the color contrast to ensure proper accessibility. |
mcoker
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.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |

What: Closes #12103
Additional issues: