Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

viewport element's width may bigger than scroll target #538

Closed
boomler opened this issue Jul 1, 2023 · 5 comments
Closed

viewport element's width may bigger than scroll target #538

boomler opened this issue Jul 1, 2023 · 5 comments

Comments

@boomler
Copy link

boomler commented Jul 1, 2023

Describe the bug

viewport element's width may bigger than scroll target

To Reproduce
open sandbox: https://codesandbox.io/s/mystifying-sunset-37zn3p?file=/src/App.js

Expected behavior

expect .os-viewport's width to be 230
expect text's ellipsis to be fully displayed.

What happened

  1. .os-viewport's width is 260
  2. text's ellipsis cannot fully displayed
image
@KingSora
Copy link
Owner

KingSora commented Jul 1, 2023

@boomler Good day! :)

Thanks for the report - The temporary fix would be to apply min-width: 0 to the .os-viewport element (https://codesandbox.io/s/crazy-sky-85wk3d?file=/src/styles.css). I'll take a look if I can add this fix to the next release without it breaking anything. I hope this solves your problem for now

@boomler
Copy link
Author

boomler commented Jul 2, 2023

@KingSora thanks for your quick reply!

And I am wondering why we need additional viewport element?

@gavinxgu
Copy link

gavinxgu commented Jul 4, 2023

@boomler Good day! :)

Thanks for the report - The temporary fix would be to apply min-width: 0 to the .os-viewport element (https://codesandbox.io/s/crazy-sky-85wk3d?file=/src/styles.css). I'll take a look if I can add this fix to the next release without it breaking anything. I hope this solves your problem for now

@KingSora The key point is that you use width: calc(100% + 30px) and padding: 15px at the same time but use box-sizing: inherit (content-box). When the box-sizing is content-box, the viewport element is always larger than the container by 30px. I think it is not a good idea to use min-width: 0. Do you consider using box-sizing: border-box instead?

By the way, I find that margin: -15px doesn't work either.

@KingSora
Copy link
Owner

KingSora commented Jul 5, 2023

And I am wondering why we need additional viewport element?

@boomler The viewport element is there for several reasons... here are a few:

  1. It hides the scrollbar even on browsers where scrollbar styling isn't supported
  2. It provides a wrapper to apply absolute padding
  3. It separates the content from other generated elements such as the scrollbars or other divs for size change detection

There are probably more, but I would say that are the most important ones.

I think it is not a good idea to use min-width: 0.

@gavinxgu Thanks for the input! :) Could you elaborate why you think min-width: 0 is not a good idea?

Do you consider using box-sizing: border-box instead?

@gavinxgu The problem here is that I don't really want to augment the box-sizing property so the user can decide what to use without really caring too much about it. Since the content is placed inside the viewport and box-sizing is a property which can be inherited it would potentially change the box-sizing values for all the content items you place inside the viewport.

The key point is that you use width: calc(100% + 30px) and padding: 15px at the same time but use box-sizing: inherit (content-box). When the box-sizing is content-box, the viewport element is always larger than the container by 30px. By the way, I find that margin: -15px doesn't work either.

The content is not always larger by 30px if the box-sizing is content-box. Just remove the white-space: nowrap rule and the content would fit perfectly.

Please don't forget that you are looking at one usecase of many possible ones. Some of the calculations may seem pointless in that usecase but are key in a different one. In fact there are so many that I've an extensive suite of end to end tests which run about 10mins to verify the functionality of the plugin for the most common ones. (this would be a new case to add to those tests).

@KingSora
Copy link
Owner

KingSora commented Jul 9, 2023

@boomler I've published overlayscrollbars v2.2.1 which includes this enhancement. Here is the full changelog: https://github.com/KingSora/OverlayScrollbars/blob/master/packages/overlayscrollbars/CHANGELOG.md#221

In case you (or @gavinxgu) have further questions please don't hesitate to ask (or to create a new issue / discussion)

@KingSora KingSora closed this as completed Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants