h2oai / wave

Realtime Web Apps and Dashboards for Python and R
https://wave.h2o.ai
Apache License 2.0
3.9k stars 323 forks source link

chore: Use maximum available space in ui.inline instead of hardcoded constant. #1974 #2152

Closed mturoci closed 9 months ago

mturoci commented 9 months ago

@marek-mihok - can you make a code review and a thorough QA to check if everything works as expected?

This is a rewrite of #1988 for #1974.

The reason some components shrunk drastically was wrong use of default align='center for case when direction='column'. This PR sets the following defaults:

marek-mihok commented 9 months ago

@mturoci, my QA efforts has continued also after the review and brought the new finding that two of our examples using ui.inline need adjustment to new changes:

image image image image

Our Tour and Wave University apps are intact. I'll further continue with my QA and let you know if something pops up.

mturoci commented 9 months ago

Perfect. Those are expected and simple to fix. Good job nonetheless. Will fix in a sec.

marek-mihok commented 9 months ago

@mturoci, I hope this is the last one - if the user sets a fixed width for the element, it should not shrink to fit the available space (in 0:04 is the correct behavior, in 0:14 it is not):

https://github.com/h2oai/wave/assets/23740173/72523d84-4bff-437c-9617-d5ac7d99df71

marek-mihok commented 9 months ago

@mturoci, the similar problem as previous one, but with fixed height of the element when inline direction is column. When height of the ui.inline is lower than its content, the content does not shrink and there is no scrollbar either, it simply visibly overflows:

image

The example from the screenshot is this one with visualization part modified:

        ui.inline(direction='column', height='400px', items=[
            ui.visualization(
                height='500px',
                plot=ui.plot([ui.mark(type='interval', x='=product', y='=price', y_min=0)]),
                data=data(fields='product price', rows=[(c, x) for c, x, _ in [f.next() for _ in range(n)]], pack=True),
            ),
            ui.vega_visualization(
                height='500px',
                specification=spec,
                data=data(fields=["a", "b"], rows=[
                    ["A", rnd()], ["B", rnd()], ["C", rnd()],
                    ["D", rnd()], ["E", rnd()], ["F", rnd()],
                    ["G", rnd()], ["H", rnd()], ["I", rnd()]
                ], pack=True),
            ),
        ]),
mturoci commented 9 months ago

The code above seems incorrect as it sets the height of the container ui.inline=(height='400px') and then wants to put there 2 500px plots, which naturally results in an overflow. Simply removing the container height should be enough.

marek-mihok commented 9 months ago

Simply removing the container height should be enough.

In that case it's all from my side. I've also checked if everything works the same way across all 3 browsers. If you don't have anything else for me to test, I'll finish my QA.

mturoci commented 9 months ago

Thanks @marek-mihok!