Skip to content

fix(drawer): fix bugs#3180

Merged
mattnolting merged 10 commits intopatternfly:masterfrom
christiemolloy:issue-2977
Jun 19, 2020
Merged

fix(drawer): fix bugs#3180
mattnolting merged 10 commits intopatternfly:masterfrom
christiemolloy:issue-2977

Conversation

@christiemolloy
Copy link
Copy Markdown
Member

@christiemolloy christiemolloy commented Jun 16, 2020

closes #2977

1 . drawer content z index variable overrides:
.pf-c-drawer__content { --pf-c-drawer__content--ZIndex: auto; }

  • It didnt seem as though z-Index on pf-c-drawer__content was doing anything so i removed it
  1. double scrollbars in pf-c-drawer__panel override:
    pf-c-drawer__panel { z-index: 10; overflow-y: hidden !important;}
  • I don’t know that we want to hide the content in the panel when it overflows, we definitely want to keep the functionality in the panel. I don’t see double scrollbars in the pf demos with drawer which makes me think that products could hide the scrollbar case by case, but i dont think thats something we want to suggest
  1. notification drawer header overlays drawer divider:
    .pf-c-drawer.pf-m-inline .pf-c-notification-drawer__header {
    // fix upstream bug where header overlays divider
    margin-left: 1px;
    }
  • I added a z-index of 200 to the ::after element that adds the "border" to the drawer component so that it matches the z-index of the notification drawer header.

@patternfly-build
Copy link
Copy Markdown
Collaborator

patternfly-build commented Jun 16, 2020

Preview: https://patternfly-pr-3180.surge.sh

Copy link
Copy Markdown
Collaborator

@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

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

Looking good, left a few comments

min-width: var(--pf-c-drawer__panel--MinWidth);
}

&.pf-m-inline {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to nest this selector

Suggested change
&.pf-m-inline {
&.pf-m-inline .pf-c-drawer__panel:not(.pf-m-no-border) {

transform: translateX(-100%);
}

&.pf-m-inline {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here

.pf-c-drawer .pf-c-drawer__panel.pf-m-no-border,
.pf-c-drawer.pf-m-panel-left .pf-c-drawer__panel.pf-m-no-border {
--pf-c-drawer__panel--BoxShadow: transparent;
--pf-c-drawer__panel--BoxShadow: 0;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
--pf-c-drawer__panel--BoxShadow: 0;
--pf-c-drawer__panel--BoxShadow: none;

@christiemolloy christiemolloy changed the title fix(notificationdrawer): fix bugs fix(drawer): fix bugs Jun 18, 2020
Copy link
Copy Markdown
Collaborator

@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

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

One last comment, then it looks good!

}

&.pf-m-inline .pf-c-drawer__panel:not(.pf-m-no-border) {
padding-right: var(--pf-c-drawer--m-panel-left--m-inline__panel--PaddingRight);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think you need to disable padding-left: 0 that's added by .pf-m-inline

Copy link
Copy Markdown
Collaborator

@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

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

LPTM! 🚀

Copy link
Copy Markdown
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.

⭐️!!!

@mattnolting mattnolting merged commit 20305a9 into patternfly:master Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants