deephaven / deephaven-plugins

Deephaven Plugins
11 stars 14 forks source link

fix: add default styling to tabs component #611

Closed AkshatJawne closed 2 months ago

AkshatJawne commented 3 months ago

Resolves #360 and resolves #476 (and also the request for a seperate styling PR made here: https://github.com/deephaven/deephaven-plugins/pull/489#issuecomment-2206860382)

Also, with regards to the snippet, as mentioned in Matt's comment, to have this properly working, we will need to have a height and width set on the view itself:

But that also depends on its parent display type and would still be broken by the view without a height set

So to have the snippet work, it would have to be something like the following:

import deephaven.plot.express as dx
import deephaven.ui as ui
from deephaven import empty_table

tabs = ui.view(
    ui.tabs(
        ui.tab_list(ui.item("My Tab", key="tab1")),
        ui.tab_panels(
            ui.item(dx.scatter(empty_table(10).update(["X = i", "Y = i"]), x="X", y="Y"), key="tab1"),
        ),
    ),
    height="100%",
    width="100%"
)
AkshatJawne commented 3 months ago

@mofojed Right, that makes sense. From my perspective, this change mainly does two things:

  1. Adds the "compact" density by default, which @dsmmcken and @mattrunyon agreed looks better -- https://github.com/deephaven/deephaven-plugins/pull/489#issuecomment-2204501724

  2. Adds the styling to the tabs component, such that if a user wanted to wrap the tabs component in a ui.view, they could have it display correctly, assuming that they have a height and width set, as I showed in the snippet above.

However, if the idea is that we want to discourage users from wrapping tabs in a ui.view, I agree, the scss is definitely not needed. May still want the density = "compact" though, @dsmmcken can confirm.

dsmmcken commented 3 months ago

I would change just compact density, other changes seems unnecessary now.

I would add some "size-100" to the tablist margin bottom by default though.

Unsure how the new tab component works, if it is a python wrapper of the tab_list, if so I would add that as a default on tab_list in python rather than JS, so it's surfaced to users as a default

AkshatJawne commented 3 months ago

Should be good now, had to set to 8px instead of size-100, given that the marginBottom on the python side expects a DimensionValue.

dsmmcken commented 3 months ago

Should be good now, had to set to 8px instead of size-100, given that the marginBottom on the python side expects a DimensionValue.

That's wrong. DimensionValue accepts a string. "size-100" should work correctly. Don't hardcode with pixel values.

AkshatJawne commented 2 months ago

Need @dsmmcken 's approval to merge