Masthead #6170
Masthead #6170
Conversation
kmcfaul
requested review from
tlabaj,
mattnolting,
mcarrano,
mcoker and
nicolethoen
|
PF4 preview: https://patternfly-react-pr-6170.surge.sh |
packages/react-core/src/components/Masthead/examples/Masthead.md
Outdated
Show resolved
Hide resolved
|
I took the Otherwise LGTM |
|
Yeah the relationship between The other option is to roll |
|
Added the expected structure to the docs |
There was a problem hiding this comment.
Looks good @kmcfaul but I did have a question about the demo you are showing. We had defined unique styles for the dropdowns placed in the masthead but I don't see those applied here. @mattnolting can you comment regarding what's needed to pull those in? They are seen in the core demo here: https://www.patternfly.org/v4/components/masthead
In my memory, keeping the two components separate left room for someone to have a clickable logo AND a non-clickable text or company name or something like that. I'm not sure if thats likely or something we really want to allow, but I'm fine either way. |
|
@mcarrano The new modifiers are in place for Dropdown/ContextSelector, but the difference in height is caused by a difference in HTML. Our I need guidance on how we want to resolve the difference, I can either override the |
|
@kmcfaul I think we need to make a styling update in Core. |
There was a problem hiding this comment.
This looks great @kmcfaul! My only nit is that, in the demo, I wouldn't expect the dropdown to be pf-m-full-height, just the context selector.
| } | ||
| isOpen={isDropdownOpen} | ||
| dropdownItems={dropdownItems} | ||
| isFullHeight |
There was a problem hiding this comment.
Don't think this would be isFullHeight
There was a problem hiding this comment.
The HTML demo has the dropdown with pf-m-full-height though?
There was a problem hiding this comment.
Yes, you're right. I should change that to dropdown with image and text. Issue 4320
packages/react-core/src/components/Masthead/MastheadContent.tsx
Outdated
Show resolved
Hide resolved
|
Will have 1 minor update when core bump gets merged to fix the toolbar demo css |
…egration demo, add full height to dropdown and context selector
|
I'm noticing that the Logo link is not navigable using the keyboard. Not sure why that is... |
|
Updated the MastheadBrand to have a tabIndex |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #6119
In this PR:
Masthead,MastheadToggle,MastheadBrand,MastheadMain,MastheadContentisFullHeightflag toDropdownandContextSelector(for demo)Open issues:
Mastheadis missinginsetmodifiers on css (core issue open) but the property is in placeToolbarContenthavingalign-items: center(which is limiting the full height). @mattnolting @mcoker Should I override the property onpf-c-toolbar__content? I noticed that this element is not in the HTML demo at all but our react component bundlespf-c-toolbar__contentandpf-c-toolbar__content-sectiontogether underToolbarContent.