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

feat: Add standard style props to UI table #921

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

mattrunyon
Copy link
Collaborator

@mattrunyon mattrunyon commented Oct 1, 2024

Fixes #606

Unfortunately, until height/width: stretch is supported in browsers, the margin keyword and its variants will push a full-width table to overflow the panel. This is how margin actually works in CSS, but most users probably actually want a padding behavior which can be achieved w/ height/width: stretch.

@mattrunyon mattrunyon requested a review from dsmmcken October 1, 2024 23:26
@mattrunyon mattrunyon self-assigned this Oct 1, 2024
Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried it, annoying that it escapes when you apply margin and would be difficult for your avg python dev to fix. I think we should re-map margin to padding in this case.

Might be able to do it via the userStyleProps named destructing?

I would add a few test cases to ui_flex.py tests to make sure we don't break it in future.

Something like:

flex_XX = ui.panel(ui.flex(ui.table(_t_flex, margin="20px"), _t_flex, direction="column"))
flex_XX = ui.panel(ui.flex(ui.table(_t_flex, margin="20px"), _t_flex, direction="row"))
flex_XX = ui.panel(ui.flex(ui.table(_t_flex, height="200px"), _t_flex, direction="column"))

@mattrunyon mattrunyon requested a review from dsmmcken October 15, 2024 23:58
@mattrunyon
Copy link
Collaborator Author

If we wanted to use padding instead of margin, we can still use Spectrum's useStyleProps by passing the view handlers instead of base style handlers. That's what I did after mapping margin to padding since React was treating explicit undefined as 0 or unset it seems when I just destructured after useStyleProps and built the style object

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is going on with flex24 webkit test? It looks different and wrong?

plugins/ui/src/js/src/elements/UITable/UITable.tsx Outdated Show resolved Hide resolved
@mattrunyon
Copy link
Collaborator Author

I think webkit is actually right on flex_24. The snapshot is 313px tall, so a 200px tall table should be ~2/3 of the height

@mattrunyon
Copy link
Collaborator Author

We could alias width/height to set both min and max width/height so the table should be fixed in size. The issue with flex_24 is it seems Chrome and Firefox use 200px and 100%=300px for the 2 elements. Then shrinks them accordingly into a 300px space. The 200px table is actually never 200px in this case because of the 100% table. As you expand the panel, the calculation shifts so the top table gets bigger, but never gets to 200px since the 100% table always includes the 200px in its desired size.

@mattrunyon
Copy link
Collaborator Author

mattrunyon commented Oct 16, 2024

Setting min/max height/width to the height/width (if the min/max is not already specified) doesn't work either. In the case where you specify a height and min-height, you will never reach the height w/ a 2nd table with defaults. Although maybe that should be expected since you specified a min-height?

@mattrunyon mattrunyon requested a review from dsmmcken October 16, 2024 19:23
Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is odd, but I guess it works

minHeight: restStyleProps.minHeight ?? restStyleProps.height,
maxHeight: restStyleProps.maxHeight ?? restStyleProps.height,
minWidth: restStyleProps.minWidth ?? restStyleProps.width,
maxWidth: restStyleProps.maxWidth ?? restStyleProps.width,

@mattrunyon
Copy link
Collaborator Author

mattrunyon commented Oct 24, 2024

This is odd, but I guess it works

minHeight: restStyleProps.minHeight ?? restStyleProps.height,
maxHeight: restStyleProps.maxHeight ?? restStyleProps.height,
minWidth: restStyleProps.minWidth ?? restStyleProps.width,
maxWidth: restStyleProps.maxWidth ?? restStyleProps.width,

Basically decided if the user sets an explicit height they probably want for it to just be that height. With the way grid determines its height and the default flex wrapper/grow/shrink, this makes it so the grid shouldn't grow/shrink in these cases unless the user also adds a min/max width/height. If neither is set and width/height aren't set then it defaults to no min/max.

The alternate is to set flex grow/shrink on the table based on props set by the user. But they could be in a non-flex environment, so I thought this might be more robust if not in a flex environment.

@mattrunyon mattrunyon merged commit 46f236e into deephaven:main Oct 24, 2024
16 checks passed
@mattrunyon mattrunyon deleted the ui-table-style-props branch October 24, 2024 19:50
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.

ui.table support for standard additional spectrum props (height, width, flex_shrink etc)
2 participants