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

Add overflow: hidden to the default wrapper styles #269

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Andarist
Copy link

It's obviously just a proposal - I've worked around my problem using wrapperClassName prop.

I can't find a strong argument against adding this - maybe you would have one. It seems to me that this is the expected behavior in the vast majority of scenarios. In fact, I don't think that anything in the editor itself can actually overflow this container... but Monaco creates a fixed-size element that gets resized based on this wrapper and the internal ResizeObserver. This makes certain layouts (like specific grids) break when resizing the viewport because the wrapper doesn't always change its size with that. The fixed-size element inside might cause the grid cell to stretch beyond what has been "reserved" for it - this SO answer contains more details about the behavior: https://stackoverflow.com/questions/43311943/prevent-content-from-expanding-grid-items/43312314#43312314

So it seems to me that this shouldn't actually break any usage in the wild while making certain grid layouts (and flex ones as well) working better out of the box.

@Andarist
Copy link
Author

After continuing the work with my fix I've noticed that Monaco tooltips can get cut off with this applied - which makes sense since they are, unfortunately, not positioned using fixed positioning. So I had to switch to using min-height: 0 to fix my original problem but applying this automatically in the library here seems to be dangerous. Unless you think otherwise - this PR could be closed.

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.

1 participant