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: allow overflow by default on ui.panel #896

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dsmmcken
Copy link
Contributor

@dsmmcken dsmmcken commented Sep 19, 2024

Fixes #178

This improves the default layout experience across a number of layout cases. The primary goal was to allow overflow auto to happen on ui.panel by default while still filling space when passing multiple objects such as tables and plots.

Tested across the cases in ui_flex.py, added e2e tests for those cases.

Changes are as follows:

  • ui.panel has an overflow property, and defaults to auto
  • ui.grid now has default gap and flex auto
  • iris-grid has a min height of 70px to improve cases where it would normally shrink to 0
  • ui panel continues to wrap children by default in a flex with align start, but immediate flex children and grid have align-self stretch which is the normal default of a flex element
  • iris grid canvas has position absolute set so it doesn't actually impact flow

BREAKING CHANGE: ui.panel now has overflow auto set by default and ui.grid now has default padding

mofojed
mofojed previously approved these changes Sep 20, 2024
Comment on lines +183 to +189
// classes can be used for deephaven ui specific css
// "deephaven.ui.components.Grid" -> "dh-grid"
if (shouldAddClassName.has(newElement[ELEMENT_KEY])) {
props.UNSAFE_className = `${props.UNSAFE_className ?? ''} ${`dh-${
newElement[ELEMENT_KEY].split('.').pop()?.toLowerCase() ?? ''
}`}`.trim();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should just make Grid.tsx and Flex.tsx components instead that handle this

The shouldWrapTextChildren also probably belongs in wrapper components instead of this file. There's not really any reason we should be doing any manipulation here IMO. It hides what actually happens with the components

The contextualHelp is an exception I guess since we're wanting any prop called contextualHelp to be modified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants