-
Notifications
You must be signed in to change notification settings - Fork 375
docs full page mastheads toolbar formatting fix #8826
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://patternfly-react-pr-8826.surge.sh A11y report: https://patternfly-react-pr-8826-a11y.surge.sh |
| ]; | ||
| const headerToolbar = ( | ||
| <Toolbar> | ||
| <Toolbar isFullHeight> |
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 believe the Dropdowns themselves also need the isFullHeight modifier on them to make the height of the user dropdowns consistent between 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.
I think that's like line 874
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.
It does look to make any visual change and 7/12 examples for username dont have that (searched for ned username and john smith) so should we change all example to have it or remove it from existing 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.
@mcarrano
Should the user dropdown in mastheads in our demos always be full height? Looks like in the react workspace right now, only half of them are full height.
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, I believe so, i.e. when they are open, the selected highlight is at the bottom of the masthead.
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 I see, I also counted the examples for pageHeader.
Added isFullHeight
thatblindgeye
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.
Other than the above convo, looks good!
mcarrano
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 good to me. Thanks @MariaAga
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #8797
Checked all other full page demos and they look fine