feat(sidebar): added padding and border to sidebar #5221
Conversation
|
Preview: https://patternfly-pr-5221.surge.sh A11y report: https://patternfly-pr-5221-a11y.surge.sh |
mattnolting
force-pushed
the
feat-sidebar-5131
branch
from
6083fe6
to
b24f503
Compare
mattnolting
force-pushed
the
feat-sidebar-5131
branch
from
b24f503
to
ed6e853
Compare
There was a problem hiding this comment.
The padding looks great. The border is triggering unnecessary scroll in all directions because of the negative offset applied based on the gutter size.
I think there are a handful of ways to handle this, and mentioned some offline. AFAIK this isn't a pressing issue (I opened it), and I think it might be worth reconsidering the approach and doing so with v5 since I think we'll likely change the gutter system anyways and probably need to rework this anyways. The same goes for the padding, potentially. WDYT @mcarrano, do you see a need to get this in for v4 (or some part of it - like the padding and/or border between panel and content), or would it be ok to defer to v5?
mattnolting
force-pushed
the
feat-sidebar-5131
branch
from
ed6e853
to
ee4b29a
Compare
|
I'm still seeing a phantom scrollbar I think? 2022-12-20_13-10-47.mp4 |
mattnolting
force-pushed
the
feat-sidebar-5131
branch
4 times, most recently
from
7a09bb6
to
c340224
Compare
mattnolting
force-pushed
the
feat-sidebar-5131
branch
from
c340224
to
f41ffc7
Compare
mattnolting
force-pushed
the
feat-sidebar-5131
branch
from
f41ffc7
to
aad0ed0
Compare
| &.pf-m-padding { | ||
| padding: var(--pf-c-sidebar__content--PaddingTop) var(--pf-c-sidebar__content--PaddingRight) var(--pf-c-sidebar__content--PaddingBottom) var(--pf-c-sidebar__content--PaddingLeft); | ||
| } |
There was a problem hiding this comment.
These vars and the .pf-m-padding vars for __panel should probably be set to 0 by default, then the .pf-m-padding declares those vars like --sidebar__content--PaddingTop: var(--sidebar__content--m-padding--PaddingTop)?
| &::before { | ||
| display: var(--pf-c-sidebar__main--before--Display); | ||
| } | ||
|
|
||
| &.pf-m-border::before { | ||
| flex: 0 0 var(--pf-c-sidebar__main--m-border--before--BorderWidth); | ||
| align-self: stretch; | ||
| order: var(--pf-c-sidebar__main--m-border--before--Order); | ||
| content: ""; | ||
| background-color: var(--pf-c-sidebar__main--m-border--before--BorderColor); | ||
| } |
There was a problem hiding this comment.
Could you move display into the second selector?
mattnolting
force-pushed
the
feat-sidebar-5131
branch
2 times, most recently
from
1c21660
to
df9e909
Compare
mattnolting
force-pushed
the
feat-sidebar-5131
branch
from
df9e909
to
7aa977a
Compare
|
🎉 This PR is included in version 5.0.0-alpha.30 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* feat(sidebar): added padding and border to sidebar * feat(sidebar): rebased, updated hbs template * feat(sidebar): updated padding mods
* feat(sidebar): added padding and border to sidebar * feat(sidebar): rebased, updated hbs template * feat(sidebar): updated padding mods
closes #5131