fix(Page) allow change of main tag

[Dominik-Petrik]

What: Closes #9000

[patternfly-build]

Preview: https://patternfly-react-pr-9296.surge.sh

A11y report: https://patternfly-react-pr-9296-a11y.surge.sh

[jenny-s51]

This is looking good Dominik! Looks like we just need to pass mainTag to the props object in the render method, since we don't want mainTag showing as an attribute on div.pf-v5-c-page

Also, I wouldn't block over this but noticed we use the prop name component for similar cases throughout the repo... maybe we can rename to include 'component' eg mainTagComponent or mainComponentTag. Does anyone have any thoughts on this?

[Dominik-Petrik]

This is looking good Dominik! Looks like we just need to pass mainTag to the props object in the render method, since we don't want mainTag showing as an attribute on div.pf-v5-c-page

Also, I wouldn't block over this but noticed we use the prop name component for similar cases throughout the repo... maybe we can rename to include 'component' eg mainTagComponent or mainComponentTag. Does anyone have any thoughts on this?

You're right I forgot to include the prop in render. And for the prop name, I'd go with mainTagComponent or maybe just mainComponent. Thanks for the feedback.

[jenny-s51]

LGTM

[thatblindgeye]

Nit: we could just call this prop component, and update the description to say Base HTML component used for the page. Defaults to 'main' or something along those lines. Could also be worth mentioning that "Only pass in 'div' if another 'main' element already exists" EDIT: just saw Jenny's comment about the same thing 😆

There could also be an argument to just typing this as a string/keyof JSX.IntrinsicElements (like some other `component' props) as who knows whether just main and div would cover whatever use cases consumers need, but at the same time that might be too flexible. Personally I think limiting the type here is fine (we should be able to update this to be more flexible if needed without a breaking change), but curious if anyone else has any thoughts.

[mcoker]

I might argue keeping it as mainComponent is good because

• There are a handful of props that are named main[PropName], so it's more consistent
• If we did ever want to allow someone to change div.pf-[v]-c-page, it's probably best if that was done via component so we could run into namespacing issues

What's the reasoning for changing it to component?

[thatblindgeye]

I was probably thinking of mainComponent as in the main tag, rather than "main" as in the base/primary/etc component. So "mainComponent" sounded a little off/confusing since the main element is one of the two types allowed. The naming is something we should align on, though, since we have a mix of "mainComponent" and just "component" for a prop.

I don't feel super strong about it, though, so if we'd want to revert this back I can push a quick change

[tlabaj]

I would agree with Michael here. I think we usually us component to change the outermost tag. Using mainComponet is more specific as to which tag/component is actually being updated.

[jenny-s51]

+1, I like mainComponent here as well

[mcoker]

@thatblindgeye ah, gotcha! I believe the word here "main" refers to the BEM element name - .pf-[v]-c-page__main. If it were its own component, we would likely call it <PageMain> based off of the BEM name.

[mcoker]

LGTM! I had to verify locally since surge wasn't up to date, even though it was finished building.

Also I left a comment about keeping the prop as mainComponent, though it's just my opinion - I'm fine with whatever y'all decide.

[tlabaj]

I think think we want to change the prop name to what you had originally.