Automatically wrap non-layout components in box containers #804
Conversation
|
Your Render PR Server URL is https://toolpad-pr-804.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cbtur14gqg4a53l0inqg. |
oliviertassinari
requested a deployment
to
default-component-boxes - toolpad-db PR #804
Render
Abandoned
apedroferreira
force-pushed
the
default-component-boxes
branch
from
e601911
to
d054d39
Compare
apedroferreira
force-pushed
the
default-component-boxes
branch
from
e6e4147
to
023801f
Compare
packages/toolpad-core/src/types.ts
Outdated
| * the component it will forward it to this property. | ||
| * Enables controlling the aligment of the component container box. | ||
| */ | ||
| hasLayoutBoxAlign?: boolean; |
There was a problem hiding this comment.
Which components don't we want this for?
There was a problem hiding this comment.
DataGrid, for example, as it takes up the whole box?
(btw i noticed bindings are still broken and fixing it now, sorry about that)
There was a problem hiding this comment.
This is public API, I'm not sure about the interface
hasLayoutBoxAlign
hasLayoutBoxJustify
- It's a bit verbose for the current use-case. e.g. might as well get away with something like a
layout: 'static' | 'flex'to mark whether a component that sizes with its box, or has a static size. - Would it make sense to be more explicit about direction? i.e. use vertical/horizontal or something instead of "align" and "justify".
layout: { vertical: 'flex', horizontal: 'static' } - It's not powerful enough to build upon. e.g. it might depend on component state. like a TextField that is in mode
fullWidthmay want to have its justification disabled.
resizableHeightProp
I'm not sure we should rely on a custom component correctly implementing the height prop. Can't we remove this resizableHeightProp and instead allow changing the height of the box container of every component?
There was a problem hiding this comment.
just fixed the bindings, i hadn't messed with bindings before but seems good now
There was a problem hiding this comment.
🤔 I'm not sure whether we should make these bindable, at least the height prop shouldn't be I think, it would create too much layout shift.
There was a problem hiding this comment.
great points, i agree with all of them
- something simpler like the
layoutprop you mentioned should cover all our current use cases, no point yet even in align/justify being separate things, i'll change it to something like that - justify/align naming is being confusing even to me so i'll change them to something more explicit
- about making the resizable height be the height of the container box, now that i abstracted the box container and am creating more "layout" props this should be easier to do and makes complete sense - this i probably will do in a separate PR though
There was a problem hiding this comment.
🤔 I'm not sure whether we should make these bindable, at least the height prop shouldn't be I think, it would create too much layout shift.
I ended up doing it because i noticed bindings were setting the default values in the controls. I don't see any cons to doing it for the alignment/justify even if it's not super useful (even though these might just be temporary controls until we allow for controlling margins a bit better).
I can make the height not be bindable though - as elements in the same row always take up the same height anyway i guess it shouldn't be too useful...
There was a problem hiding this comment.
Some other options
{
flex: 'vertical' | 'horizontal' | 'both',
flex: 'row' | 'column' | 'all',
flex: 'x' | 'y' | 'xy'
}Letting the imagination free, we could in the future expand that to (props) => 'vertical' | 'horizontal' | 'both'. e.g.
{
flex: ({ fullWidth }) => fullWidth ? 'vertical' : 'both'
}There was a problem hiding this comment.
Some other options
{ flex: 'vertical' | 'horizontal' | 'both', flex: 'row' | 'column' | 'all', flex: 'x' | 'y' | 'xy' }Letting the imagination free, we could in the future expand that to
(props) => 'vertical' | 'horizontal' | 'both'. e.g.{ flex: ({ fullWidth }) => fullWidth ? 'vertical' : 'both' }
we can make it a function that takes the props, just not sure how useful it would really be to make this behavior change according to stateful values.
i wouldn't mind that a "horizontal alignment" control was enabled even if a component was taking up the full-width - wouldn't be wrong, it just wouldn't be doing anything in that particular case.
but there's probably in no harm in making this more powerful in case we see the need for it down the line.
|
@Janpot how about this? |
| elm.layout?.[propName as keyof typeof layoutBoxArgTypes] || | ||
| appDom.createConst(argType?.defaultValue ?? undefined); | ||
| const bindingId = `${elm.id}.layout.${propName}`; | ||
| const scopePath = `${elm.name}.@layout.${propName}`; |
There was a problem hiding this comment.
i want to make sure this scope path can't collide with any normal prop name. any other suggestions?
There was a problem hiding this comment.
It shouldn't be in the scope, you can remove the scope path. In fact, should we even have these bindable?
There was a problem hiding this comment.
ok let's make these not be bindable for now - i'll just have to set the default value in a different way if i remember correctly
|
merging, if there are any more suggestions i can add later. |
Issue #712
Not 100% convinced about this UI for the box alignment (which might just be a temporary thing until we improve resizing anyway), but just looking for feedback on where to place it, or this might be okay after all.