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

Update @render.data_frame renderer to work properly with fill/fillable #1126

Merged
merged 12 commits into from
Feb 27, 2024

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Feb 14, 2024

Closes #1118

@cpsievert cpsievert marked this pull request as draft February 14, 2024 00:06
@cpsievert cpsievert marked this pull request as ready for review February 14, 2024 21:25
js/dataframe/index.tsx Outdated Show resolved Hide resolved
@jcheng5
Copy link
Collaborator

jcheng5 commented Feb 20, 2024

Overall this looks great! Only regressing case I could find is height="auto" in a non-fill context. There's a ton of extra space now that wasn't there before.

import seaborn as sns

from shiny.express import render, ui

df = sns.load_dataset("iris").iloc[:5,]

@render.data_frame
def data_frame():
    return render.DataGrid(
        df,
        height="auto",
    )

with ui.p():
    "Hello"

@jcheng5
Copy link
Collaborator

jcheng5 commented Feb 20, 2024

I also noticed that even on main, output_data_frame always makes the outermost element fill/fillable, which I don't think is correct if height="auto" is passed. Like if you take the example from my previous comment, and wrap all the UI in a ui.card(height="800px").

@cpsievert
Copy link
Collaborator Author

cpsievert commented Feb 23, 2024

I don't love the centering. Do we do that anywhere else?

Agreed (and I don't think so) in general, but it's maybe worth doing inside a card? 87daed7 restricts it to just a .card-body context, but I can probably be talked into not doing that.

There's a ton of extra space now that wasn't there before.

Oops, that's cause of the flex-basis I added last minute without fully thinking it through. 87daed7 reverts that.

I also noticed that even on main, output_data_frame always makes the outermost element fill/fillable

I don't think this is a problem, at least for the situation you're thinking of. It could be a problem though if the children of that element are for some reason not compatible with a display:flex; flex-direction:column layout.

Just to illustrate, here's the example you have in mind, with the result (which seems right):

import seaborn as sns

from shiny.express import render, ui

df = sns.load_dataset("iris").iloc[:5,]

with ui.card(height="400px"):
    @render.data_frame
    def data_frame():
        return render.DataGrid(df, height="auto")

with ui.p():
    "Hello"
Screenshot 2024-02-23 at 3 06 29 PM

@jcheng5
Copy link
Collaborator

jcheng5 commented Feb 26, 2024

I actually meant this:

import seaborn as sns

from shiny.express import render, ui

df = sns.load_dataset("iris").iloc[:5,]

with ui.card(height="400px"):
    @render.data_frame
    def data_frame():
        return render.DataGrid(df, height="auto")

    with ui.p():
        "Hello"
image

If you drop the fill classes from the ui.output_data_frame's outermost element, you get this instead (which I think is better):

image

@jcheng5
Copy link
Collaborator

jcheng5 commented Feb 26, 2024

We can merge without that fix though, if you don't think there's anything we can do about it right now.

@cpsievert
Copy link
Collaborator Author

Ah, good point. 789147c addresses that.

@jcheng5
Copy link
Collaborator

jcheng5 commented Feb 26, 2024

Oh, interesting approach! Great.

Still feels weird to me that it's centered, but like you, I could be talked into it. Have you run this by Garrick or Greg?

@cpsievert
Copy link
Collaborator Author

@gregswinehart "loves it" :)

@jcheng5
Copy link
Collaborator

jcheng5 commented Feb 27, 2024

OK, I'm happy to be overruled!

@cpsievert cpsievert merged commit 2c1c32c into main Feb 27, 2024
26 checks passed
@cpsievert cpsievert deleted the fix-dataframe-fill branch February 27, 2024 01:22
schloerke added a commit that referenced this pull request Feb 27, 2024
* main:
  Update `@render.data_frame` renderer to work properly with fill/fillable (#1126)
  Don't error on malformed date input (#1139)
  fix(nav_panel): Add `.bslib-gap-spacing` if nav panel is fillable (#1154)
  feat: Add `ui.input_dark_mode()` (#1149)
  chore(make): Add `make docs` and `make docs-preview` (#1150)
  Sync with latest bslib sidebar and card changes (#1129)
  docs(input_task_button): Add to reference index, also update methods for action button/link (#1151)
  chore(make): Update check and check-fix to replicate CI (#1148)
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.

Modify data grid to work well with fill/fillable
3 participants