Skip to content
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(Panel): added Penta styles #6741

Merged
merged 6 commits into from
Jun 18, 2024
Merged

Conversation

thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Jun 4, 2024

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

@patternfly-build
Copy link

patternfly-build commented Jun 4, 2024

@thatblindgeye thatblindgeye linked an issue Jun 6, 2024 that may be closed by this pull request
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • --#{$panel}--m-bordered--before--BorderWidth: var(--pf-t--global--border--width--regular);
    • I wonder if this should match card, which uses --pf-t--global--border--width--box--default
  • We've removed z-index from things that popper controls (menus, tooltips, popovers, etc), so I'd be in favor of removing it here, too. z-index is on things that PF CSS positions like drawers and sticky things
    --#{$panel}--m-raised--ZIndex: var(--pf-t--global--z-index--sm);
  • 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}--m-scrollable__footer--BoxShadow: 0 #{pf-size-prem(-5px)} #{pf-size-prem(4px)} #{pf-size-prem(-4px)} rgba(0 0 0 / 16%); // insets the shadow so it doesn't show on left/right sides

@@ -7,6 +7,7 @@
--#{$panel}--ZIndex: auto;
--#{$panel}--BackgroundColor: var(--pf-t--global--background--color--primary--default);
--#{$panel}--BoxShadow: none;
--#{$panel}--BorderRadius: var(--pf-t--global--spacer--md);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to match menu, it uses the small border radius

Suggested change
--#{$panel}--BorderRadius: var(--pf-t--global--spacer--md);
--#{$panel}--BorderRadius: var(--pf-t--global--border--radius--small);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree with @mcoker's points above! i'm down to match this with menu!

@thatblindgeye
Copy link
Contributor Author

@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?

@mcoker
Copy link
Contributor

mcoker commented Jun 10, 2024

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

@mcoker mcoker requested a review from srambach June 11, 2024 19:25
Copy link

@lboehling lboehling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gorgeous!

Copy link
Member

@srambach srambach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👨‍🍳 looks great :)

@@ -107,6 +110,13 @@
padding-block-end: var(--#{$panel}__main-body--PaddingBlockEnd);
padding-inline-start: var(--#{$panel}__main-body--PaddingInlineStart);
padding-inline-end: var(--#{$panel}__main-body--PaddingInlineEnd);

&:has(.#{$panel}__menu) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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__block with component__block-body as a child (or children if you have multiple sections). So I'd probably expect __menu to 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>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the demo to just remov ethe MainBody, but the panel menu is still rendered inside the PanelMain component

### With a menu

```hbs
{{#> panel}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super duper nit, would you mind updating this demo to the riased variant? I imagine that's the most common use case.

Suggested change
{{#> panel}}
{{#> panel panel--modifier="pf-m-raised"}}

@mcoker mcoker merged commit c36a7a1 into patternfly:v6 Jun 18, 2024
4 checks passed
@mcoker mcoker mentioned this pull request Jun 18, 2024
@patternfly-build
Copy link

🎉 This PR is included in version 6.0.0-alpha.165 🎉

The release is available on:

Your semantic-release bot 📦🚀

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panel - Penta styling
5 participants