-
Notifications
You must be signed in to change notification settings - Fork 376
feat(Compass): updated mainheader structure to be composable #12135
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
Conversation
|
Preview: https://pf-react-pr-12135.surge.sh A11y report: https://pf-react-pr-12135-a11y.surge.sh |
kmcfaul
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.
Overall looking good to me, just had one comment about reusing the new components internally and it'll need a rebase once the Hero PR goes in!
| import { css } from '@patternfly/react-styles'; | ||
|
|
||
| interface CompassPanelProps extends React.HTMLProps<HTMLDivElement> { | ||
| export interface CompassPanelProps extends React.HTMLProps<HTMLDivElement> { |
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.
Doh! That may be why some the props tables were broken
| <div className={css(`${styles.compass}__main-header-content`)}> | ||
| {title && <div className={css(`${styles.compass}__main-header-title`)}>{title}</div>} | ||
| {toolbar && <div className={css(`${styles.compass}__main-header-toolbar`)}>{toolbar}</div>} | ||
| </div> |
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 think we should use the new CompassMainHeaderContent/CompassMainHeaderTitle/CompasMainHeaderToolbar here instead of creating the divs again. But not a blocker
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.
Whoops
13acd8d to
aad0234
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.
LPTM!
What: Closes #12108
Will need to wait on #12131 going in first; made updates to CompassHero just to appease the precommit checks.
Additional issues: