refactor(dashboard): measure scrollbar width directly from element - #2069
chintankavathia wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request deprecates the ScrollbarHelper service and updates SiDashboardComponent to calculate scrollbar width directly using DOM properties. Feedback suggests refining the calculation to account for borders, ensuring SSR safety by guarding DOM access with isPlatformBrowser, and optimizing performance in the dashboard component by utilizing existing dimension data to avoid redundant DOM reads.
4e0397e to
70ec30a
Compare
| let padding = this.document.body.offsetWidth >= BOOTSTRAP_BREAKPOINTS.mdMinimum ? 32 : 16; | ||
| if ( | ||
| dashboardDimensions && | ||
| dashboardFrameDimensions && | ||
| dashboardDimensions.height > dashboardFrameDimensions.height | ||
| ) { | ||
| padding = padding - this.scrollbarHelper.width; | ||
| const { offsetWidth, clientWidth } = this.dashboardFrame().nativeElement; | ||
| padding = padding - (offsetWidth - clientWidth); |
There was a problem hiding this comment.
I think this whole padding calculation stuff can be dropped completely. Instead use the normal padding helper classes and when the scrollbar is visible, trigger a class that has scrollbar-gutter: stable;. What scheduler uses these days
There was a problem hiding this comment.
May be I don't get it but isn't here it is opposite so we need to decrease padding when scrollbar is present but scrollbar-gutter: stable; ensures to have extra space always even when there is no scrollbar. padding value needs to be decreased upto width of scrollbar so that it vertically aligns with button above which differs browsers to browsers.
There was a problem hiding this comment.
Ah...didn't really read the code. I have a similar use case with a header and a scrollable area. What I do is to add the scrollbar-gutter to the header so it is aligned with the scroll area
There was a problem hiding this comment.
That would work in terms of alignement but will add extra space when we have scrollbar:
with proposed change
- with scrollbar
-without scrollbar
existing
- with scrollbar
- without scrollbar
@panch1739 WDYT
|
@chintankavathia please rebase |
9d42b12 to
b1bee80
Compare
Replace ScrollbarHelper service with direct offsetWidth - clientWidth measurement on the scrollable element, which is more accurate and SSR-safe. DEPRECATED: `ScrollbarHelper` service is deprecated and will be removed in v52. Use `element.offsetWidth - element.clientWidth` on the scrollable element instead.
b1bee80 to
b7db398
Compare
3 participants
Replace ScrollbarHelper service with direct offsetWidth - clientWidth measurement on the scrollable element, which is more accurate and SSR-safe.
DEPRECATED:
ScrollbarHelperservice is deprecated and will be removed in v52. Useelement.offsetWidth - element.clientWidthon the scrollable element instead.Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: