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(Page): Deprecate PageHeader component #8854
Conversation
Preview: https://patternfly-react-pr-8854.surge.sh A11y report: https://patternfly-react-pr-8854-a11y.surge.sh |
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.
In addition the the file comments below:
- In the Page.md file, the
Uncontrolled navigation" example still mentions
<PageHeader` in the example description. - The "Flyout nav" demo in the Nav.md file has PageHeader and PageHeaderTools imported from react-core still
packages/react-core/src/components/Page/examples/PageVerticalNavUsingPageHeaderComponent.tsx
Show resolved
Hide resolved
@@ -119,3 +112,10 @@ The content in this example is placed in a card to better illustrate how the sec | |||
|
|||
```ts file="./PageCenteredSection.tsx" | |||
``` | |||
|
|||
### Deprecated page header |
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 we move this example into it's own file in the deprecated folder along with the PageHeader. That way it would show up under a deprecated tab. That way the non recommend solution is not shown on the main Page
page.
cc @mcarrano what do you think?
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.
@tlabaj I think so, but let me make sure I understand. Are you saying that when the user goes to the Page component, they will then see two React tabs? One with the masthead examples and another called "React deprecated" that has the old example using the PageHeader? I guess that's OK.
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.
Yes. That is what I am saying. This one example using the PageHeader would be in the React deprecated
tab.
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.
Then yes, I think that works.
6f605f2
to
7f7ddea
Compare
propComponents: ['PageHeader','PageHeaderTools','PageHeaderToolsGroup','PageHeaderToolsItem'] | ||
--- | ||
|
||
## PageHeader |
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 we change to "Page header"
7f7ddea
to
6a70b52
Compare
propComponents: ['PageHeader','PageHeaderTools','PageHeaderToolsGroup','PageHeaderToolsItem'] | ||
--- | ||
|
||
## PageHeader examples |
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 we update this to "Page header examples"
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.
@tlabaj done
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 should be able to move away from the /dist/esm/deprecated
imports, but I wouldn't block over it.
@nicole can you address Austin's comment when you resolve conflicts? |
79678f3
to
924ee27
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.
Looks good to me!
What: Closes #8602