iTwin / appui

Monorepo for iTwin.js AppUi
MIT License
8 stars 2 forks source link

Properly manage zero width/height for viewport components #608

Open a-gagnon opened 11 months ago

a-gagnon commented 11 months ago

Is your feature request related to a problem? Please describe. For civil, we use ViewportComponent and FloatingViewportContent react components. We have our homebrew solution to make sure those components are not rendered unless they have a valid width or height (>0) because it throws an error in core's ScreenViewport.create.

Describe the solution you'd like It would be nice if appui could enforce positive viewport width/height, or replace by a simple

when it's null.

raplemie commented 11 months ago

Thanks, we'll look into this!

raplemie commented 10 months ago

Hi @a-gagnon, a fix for this was released in 4.7.1 today, viewport will now wait to have a valid size before mounting and setting the viewport ref, let us know if that addresses your issue!

a-gagnon commented 10 months ago

We had code that assumed when viewport size went to 0,0 (or had an area of 0), the viewport component would close and we would get called on IModelApp.viewManager.onViewClose event. It is no longer the case.

There is also an issue with our resize observer when using popout and then back into a panel. The resize observer doesn't get called anymore. Most likely related to #524 .

Question: what does it mean to have a viewport with size 0. Should that be allowed at all? @pmconne @markschlosseratbentley Any thoughts about opening a viewport with a valid size then resizing a viewport to size 0 after creation? The error in itwinjs-core does not get triggered

raplemie commented 10 months ago

Changing that would likely be an unexpected behavior change (viewport completely unmounting) although I admit that the end result of my fix makes this behavior unexpected, we'll discuss internally but I'll probably remove this behavior in a patch, as this puts us in a weird state of "half managed" behavior, which is not right either.

raplemie commented 10 months ago

Hi @a-gagnon, since the resulting state was wrong, this behavior was reverted in 4.7.2 to what it was in 4.7.0. Until we find a proper solution, I suggest keeping your resize observer solution.