search

carbon-design-system / ibm-products

A Carbon-powered React component library for IBM Products
https://ibm-products.carbondesignsystem.com
Apache License 2.0
98 stars 138 forks source link

Pageheader is not reliably calculating top offset #4427

Closed xmannyxfreshx1 closed 3 months ago

xmannyxfreshx1 commented 9 months ago

What package(s) are you using?

Detailed description

Describe in detail the issue you're having.

Is this issue related to a specific component?

What did you expect to happen? What happened instead? What would you like to see changed?

What browser are you working in?

What version of the @carbon/ibm-products (or @carbon/ibm-cloud-cognitive) package are you using?

What offering/product do you work on? Any pressing ship or release dates we should be aware of?

CP4AIOps. Multiple squads are reporting similar behavior using the same Pageheader components.

Please create a reduced test case in CodeSandbox

Additional information

xmannyxfreshx1 commented 9 months ago

@Ratheeshrajan I believe these were your changes https://github.com/carbon-design-system/ibm-products/pull/4068

matthewgallo commented 6 months ago

Hey @xmannyxfreshx1, I'm not able to access the code sandbox link you shared. Can you make sure that it is public? We'd like to look more into this issue and the code sandbox reproduction would really help. Thanks!

matthewgallo commented 6 months ago

This code sandbox also demonstrates the issue. Thanks @richardpilot! https://codesandbox.io/p/live/c0c44383-6add-4fe8-b1e4-1ef8ee52c412?file=%2Findex.html

richardpilot commented 6 months ago

Updated sandbox: https://codesandbox.io/p/devbox/thirsty-nash-d62645

xmannyxfreshx1 commented 6 months ago

@matthewgallo I've made the sandbox public Pageheader not collapsing, as expected, under Carbon product header

davidmenendez commented 4 months ago

@richardpilot @xmannyxfreshx1 thanks for opening this issue. sorry this took so long to get to.

i've been able to solve this problem with making some adjustments to the implementation. namely by making two changes to the css

.App { // NEW
  height: 100vh;
  overflow: hidden;
}

.main--content {
  height: calc(100% - #{$spacing-09});
  margin-top: $spacing-09;
  overflow-y: auto; // NEW
}

in order to get everything to calculate correctly the div that wraps the PageHeader and Header (in this case App) needs to have a height and overflow set. then adding the overflow control to the content. if you look at this sandbox you'll be able to see how it should be implemented.

i apologize if this isn't exactly the most intuitive solution. after looking more at this component i think there's some room for improvement that would improve some quality of life like not having to implement all these heights and overflows. the current storybook implementation doesn't exactly demonstrate this extra required boilerplate.

i'm going to go ahead and close this for now. if you have any other questions please feel free to reach out or re-open this issue.

richardpilot commented 4 months ago

@davidmenendez Unfortunately that solution doesn't work. The only reason it works for the sample is because it has explicit heights set. Remove those and let the content naturally overflow and the content no longer scrolls because you explicitly set the overflow to hidden.

https://codesandbox.io/p/devbox/strange-morse-k7g86x?file=%2Fsrc%2FExample%2FExample.jsx%3A66%2C17&workspaceId=7bd97f32-1182-43fb-914c-3018a28d33e1

davidmenendez commented 4 months ago

@richardpilot ahh i see. yes that is a problem. we will take another look at this.

i am unable to access that codesandbox though. can you please make it public? thanks!

richardpilot commented 4 months ago

@davidmenendez Hopefully this one works! https://codesandbox.io/p/live/38891610-9938-42dc-b9c7-fe483ac6ac9f

richardpilot commented 3 months ago

@davidmenendez Looks like the headerOffset check is wrong. It only triggered if the offsetTopMeasuringRef was at the top (i.e. 0) which has no effect and is the opposite behaviour that you would need in an offset checker.

I've inverted the check so that it works when there's an actual offset. The storybook still works with your boilerplate adjustments but now it also works for the standard Carbon Header approach