feat(Panel): added Penta styles#6741
Conversation
|
Preview: https://patternfly-pr-6741.surge.sh A11y report: https://patternfly-pr-6741-a11y.surge.sh |
There was a problem hiding this comment.
I might suggest these changes, noting where the suggestion comes from for consistency. I'd like @lboehling or @andrew-ronaldson to confirm the design related changes
-
- I wonder if this should match card, which uses
--pf-t--global--border--width--box--default
- I wonder if this should match card, which uses
- We've removed
z-indexfrom things that popper controls (menus, tooltips, popovers, etc), so I'd be in favor of removing it here, too.z-indexis on things that PF CSS positions like drawers and sticky things - Menu uses a lg spacer for left/right padding on things
- Menu has a 16px visible top/bottom padding for its search/header and footer sections, though it comes from an 8px gap on the top-level menu component, 8px top/bottom padding on the outer menu wrapper, and 8px top/bottom margin on the search and footer elements. I suppose we can match that, though I'd probably leave off the 8px top/bottom padding on the outer panel component - I believe that's specific to the inner part of a list. Though I wonder if no gap on the parent would be better and we just use 16px (md) top/bottom padding on the header/footer and menu body sections. One way I think of menu is a use case @nicolethoen has mentioned, and I think 16px top/bottom padding, 24px left/right padding works everywhere except menu, which could just drop into the panel or go into a panel__menu wrapper that has no padding:
--------- | header ---------- | [menu component, maybe with a panel__menu wrapper] ---------- | footer ---------- - Footer uses a custom box-shadow, menu uses
--pf-t--global--box-shadow--md--top
| --#{$panel}--ZIndex: auto; | ||
| --#{$panel}--BackgroundColor: var(--pf-t--global--background--color--primary--default); | ||
| --#{$panel}--BoxShadow: none; | ||
| --#{$panel}--BorderRadius: var(--pf-t--global--spacer--md); |
There was a problem hiding this comment.
if we want to match menu, it uses the small border radius
| --#{$panel}--BorderRadius: var(--pf-t--global--spacer--md); | |
| --#{$panel}--BorderRadius: var(--pf-t--global--border--radius--small); |
There was a problem hiding this comment.
Oop yeah this should be the border--radius token. @lboehling do we want to match Menu or keep the medium border radius per discussion during working session?
There was a problem hiding this comment.
i agree with @mcoker's points above! i'm down to match this with menu!
24cb8c6 to
dbfc7d9
Compare
|
@mcoker @lboehling made updates per above. I added a demo to show a Menu inside a Panel that could use some eyes. It feels like we would want to remove the box-shadow that Menu has by default if it's inside a Panel? |
@thatblindgeye the plain menu should work nicely for that |
| padding-inline-start: var(--#{$panel}__main-body--PaddingInlineStart); | ||
| padding-inline-end: var(--#{$panel}__main-body--PaddingInlineEnd); | ||
|
|
||
| &:has(.#{$panel}__menu) { |
There was a problem hiding this comment.
I wonder if we might handle this a little different for a couple of reasons
- there is a potential performance concern around using
:has() - in other components, the structure is usually something like
component__blockwithcomponent__block-bodyas a child (or children if you have multiple sections). So I'd probably expect__menuto be adjacent to__main-body
Since the react component is composable, wdyt about updating this so __menu is adjacent to __main-body? So in react it would look something like below, though I'm guessing if you're using this as a menu, you probably wouldn't use __main-body, you'd just have __menu in the main area?
<Panel>
<PanelHeader>Header content</PanelHeader>
<Divider />
<PanelMain tabIndex={0}>
<PanelMainBody>
a sentence or two
</PanelMainBody>
<PanelMenu>
/* menu component */
</PanelMenu>
<PanelMainBody>
a sentence or two
</PanelMainBody>
</PanelMain>
<PanelFooter>Footer content</PanelFooter>
</Panel>
There was a problem hiding this comment.
Updated the demo to just remov ethe MainBody, but the panel menu is still rendered inside the PanelMain component
58bca86 to
f05afdc
Compare
src/patternfly/demos/Panel/Panel.md
Outdated
| ### With a menu | ||
|
|
||
| ```hbs | ||
| {{#> panel}} |
There was a problem hiding this comment.
super duper nit, would you mind updating this demo to the riased variant? I imagine that's the most common use case.
| {{#> panel}} | |
| {{#> panel panel--modifier="pf-m-raised"}} |
f05afdc to
4905aba
Compare
|
🎉 This PR is included in version 6.0.0-alpha.165 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #6732
Worth noting that the scrollable example currently looks off. I can apply similar styling as the scrollable menu example (to make the scrollbar look like it has a curve to it), but may ultimately depend on how we want to proceed re: #6045