8000 fix(page): fixed fill/scroll with notification drawer by mcoker · Pull Request #7130 · patternfly/patternfly · GitHub
[go: up one dir, main page]

Skip to content

fix(page): fixed fill/scroll with notification drawer#7130

Merged
mcoker merged 1 commit intopatternfly:mainfrom
mcoker:issue-7127
Sep 30, 2024
Merged

fix(page): fixed fill/scroll with notification drawer#7130
mcoker merged 1 commit intopatternfly:mainfrom
mcoker:issue-7127

Conversation

@mcoker
Copy link
Contributor
@mcoker mcoker commented Sep 30, 2024

fixes #7127

@mcoker mcoker requested a review from srambach September 30, 2024 18:09
@patternfly-build
Copy link
Collaborator
8000 patternfly-build commented Sep 30, 2024

.#{$drawer}__body > .#{$page}__main {
height: 100%;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old v5 style, doesn't match the v6 structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the drawer body container being removed completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not from the component, just from the page component structure when the page-main-container/page-main elements are a child of the drawer content. AFAIK the only way to get that structure in react is via <Page notificationDrawer={...}>. So we would just remove the wrapper element <DrawerContentBody> here - https://github.com/patternfly/patternfly-react/blob/5decd6a8bf9632288abdbc98d3cac1902303c04e/packages/react-core/src/components/Page/Page.tsx#L349

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it looks like it is just in relation to the page and does not impact the drawer component.


.#{$page}__drawer > .#{$drawer} {
flex: 1 0 auto;
.#{$page}__drawer .#{$page}__main-container {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should only be one .#{$page}__main-container in .#{$page}__drawer unless anyone can think of a situation where that isn't the case? cc @srambach @mattnolting @dlabrecq

.#{$page}__drawer > .#{$drawer} {
flex: 1 0 auto;
.#{$page}__drawer .#{$page}__main-container {
align-self: revert;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

default for .#{$page}__main-container is align-self: start< 8000 /code> which pushes that element to the top of the page component's grid layout. Since .#{$page}__main-container goes in .#{$drawer}__content (a flex column), that pushes __main-container to the left. This can just be the default/stretch.

align-self: revert;

&.pf-m-fill {
flex-grow: 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default for __main-container.pf-m-fill is align-self: stretch which works great in the page grid layout. Since this is in a flex column, it can just grow.

{{#> drawer}}
{{#> drawer-main}}
{{#> drawer-content}}
{{#> drawer-content drawer-content--NoBody=true}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just removes drawer__body, so page__main-container is a child of drawer__content, a flex column. Allows us to manage the height/alignment of page__main-container better

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

Great work, LPTM 🐰

@mcoker mcoker merged commit 18a3a5a into patternfly:main Sep 30, 2024
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.

🏎️ Go! Looks awesome, good detecting

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.

🏎️ Go! Looks awesome, good detecting

dgdavid added a commit to agama-project/agama that referenced this pull request Nov 21, 2024
To avoid "stacking contexts" problems which avoid sticky page sections
behave as expected.

Such a workaround will be not needed when migrating to latest versions
of PF6, since the problem has been addressed by droping the
DrawerContentBody from the Page component, see
patternfly/patternfly#7130 and related links

See also https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_positioned_layout/Understanding_z-index/Stacking_context
@mcoker mcoker deleted the issue-7127 branch January 7, 2025 16:55
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.

Page main container with pf-m-fill doesn't work in a drawer

5 participants

0