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

Proposal: API - Move from 'style' to Compositional Components #489

Open
15 tasks
bryphe opened this issue May 11, 2019 · 10 comments
Open
15 tasks

Proposal: API - Move from 'style' to Compositional Components #489

bryphe opened this issue May 11, 2019 · 10 comments
Labels
A-components Area: Consumer-exposed components providing some convenience over the primitives A-layout Area: Layout A-primitives Area: Primitives, the basic building blocks A-rendering Area: Rendering artifacts, features etc. A-technical Area: Technical issues not directly related to features enhancement New feature or request help wanted Extra attention is needed

Comments

@bryphe
Copy link
Member

bryphe commented May 11, 2019

This came from some discussions with @manuhornung and others around Flutter's model vs our style model. IMO, one thing Flutter did really well in their design is their compositional component model (moving away from the uber-style grab bag of properties).

To clarify, what I mean by a compositional model, is instead of having a Style.[...] with arbitrary properties (like positioning, transform, etc), Flutter has more scoped widgets - like a Transform widget, or Positioned widget. This is nice in terms of discoverability - each of those widgets takes a relatively small and concise set of properties.

In addition, the Style.[..] model is coupled to a single layout system, whereas Flutter uses component composition to express a variety of layout models - this is very convenient for building UI apps.

The Style.[..] model has gotten us really far, but there are a few downsides I've seen so far:

  • Discoverability: The use of Polymorphic Variants isn't as well supported with auto-complete (as far as I can tell), whereas specific properties would be more discoverable
  • Discoverability: It's challenging to know all the potential properties, and how they interact, when using Style.[..].
  • Performance: We always have to fold our style-list into a style-object:
    let applyStyle = (style, styleRule) =>
    - I wonder if there could be a cheaper way to handle this? Merging is always challenging:
    let merge = (~source, ~target) =>
  • Performance: Our Node class ends up having to do a lot of different things - recompute transforms, even if there is are no extra transforms, check the overflow property, etc.

I think the compositional model could help with some of the discoverability, by having a more constrained set of properties per-component, and help with the performance by giving each component a more constrained set of responsibilities (potentially minimizing extra transform calculations, overflow calculations, etc).

If people are on-board with moving to the compositional model vs the uber-style model, I'd propose tackling it incrementally, by removing style properties and replacing them with compositional components:

  • The transform property could become the responsibility of Transform.Rotate/Transform.Scale, etc components ([WIP] API: Transform composite component #488)
  • The position and top/left/right/bottom could become the responsibility of Stack and Positioned components (Feature - API: Some initial compositional components #483)
  • The backgroundColor property could become the color property of a Container
  • The color property could become the color property of a Text
  • padding property could be moved to a Padding component
  • Our current flex row / column settings could be moved to Row and Column components
  • border properties could be moved to a Border component
  • opacity could be moved to an Opacity component
  • overflow could be moved to a ClipRect component
  • boxShadow could be moved to a BoxShadow component
  • cursor and mouse event handling could be moved to a MouseInput component
  • focus behavior and keyboard handling could be moved to a KeyboardInput component

In addition, this model would be amenable for future work:

  • We could model accessibility (Proposal: Accessibility #293) with compositional components
  • Gestures could use some sort of GestureDetector component (potentially plugging into @jordwalke 's prototype)
  • We could model alternative layout strategies, if it makes sense. For this, it'd be nice if we had a way to statically type the component tree (ie, what children are allowed)

The ultimate goal would be to remove the style property entirely, and represent everything it modelled via new components, instead.

Some things to think about:

  • Would each of these components need a Node in our scene-tree (aka, would they be primitive components?)? If so, they probably don't all need to generate LayoutNode's - we might need to take a look at that. Also, if we did model these as Node's - likely a lot of the 'core' behavior in the Node class could be factored across these new, smaller primitives. For example - this Overflow.render behavior would only need to be used by the new ClipRect's node.
@bryphe bryphe added enhancement New feature or request help wanted Extra attention is needed labels May 11, 2019
@bryphe
Copy link
Member Author

bryphe commented May 11, 2019

One other motivation we had for our current Style.[..] model was to facilitate interop / component-reuse between Brisk <-> Revery. I believe that switching to this compositional model would still work (and perhaps even make it simpler! As the abstract-common-building-blocks would have less surface area to implement in both sides).

@wokalski
Copy link
Collaborator

@bryphe to break it down; I like the approach of components for layout or gestures. However, some of them are semantically unclear. Things become tricky when we have a component parent and multiple children (like: <Transform> <View /> <View /> <Transform>)

  • Transform: Would a transform component apply the transform to all its children one by one? No matter how deep they are (I.e. a host nodes might be n layers below the Transform component)? Or would it mimic a view and transform children as if they were in a container? It's especially relevant in stuff like z axis or rotation transforms.
  • Position, Padding, Row, Column: Makes sense, the cases with multiple children are clear with layout containers
  • Opacity: Makes sense, too
  • Border, BoxShadow: Is it encompassing all children? If yes would it be around the minimum bounding box? Or maybe just around the elements themselves? What if they overlap?

When it comes to other use cases, I think it's a great idea. I outlined the changes needed to refactor the reconciler to support nesting different instances of "React" here

@bryphe
Copy link
Member Author

bryphe commented May 11, 2019

Thanks for the feedback @wokalski , great points.

Transform: Would a transform component apply the transform to all its children one by one? No matter how deep they are (I.e. a host nodes might be n layers below the Transform component)? Or would it mimic a view and transform children as if they were in a container?

Good point, this is tricky! Flutter solves it by making their Transform components only work with a single child. We could do something similar; we just don't have a good way to express this with the type system today (it'd have to be a runtime error).

Alternatively, we could treat a list of children as sort of a virtual container. This is important for transforms like rotation, where the default expected behavior is to rotate around the 'center' - but where is the 'center' for a group? We could create a center based on an aggregated axis-aligned bounding box of the components, and that might be OK - but not 100% sure.

Or would it mimic a view and transform children as if they were in a container?

I'm not sure what this part means - transforms would be applied up the chain (every 'component' has a transform, even though it might be an identity transform). Essentially during rendering we traverse the node-tree and apply the transforms via matrix multiplication across the train.

Border, BoxShadow: Is it encompassing all children? If yes would it be around the minimum bounding box? Or maybe just around the elements themselves? What if they overlap?

Good point again! I think this is a similar trade-off with the transform. In flutter, I guess they actually don't have these widgets - it's a decoration option on some widgets.

But I think we could have two options again:

  • Option 1: Restrict to single child
  • Option 2: Consider the aggregate axis-aligned bounding box of children, and use that to draw the shadow / border around. This seems like 'reasonable' behavior for something like:
<BoxShadow>
   <Container width=32 height=32 />
   <Container width=48 height = 48 />
</BoxShadow>

In Option 1, we'd throw a runtime error. In Option 2, we'd figure out the dimensions of each container (dependent on the layout strategy), create an aggregate axis-aligned bounding box, and then during paint, we'd render the box shadow according to the aggregate. If it's a single item - the bounding box would be the single item's bounding box.

I think, without having the type system be able to validate at compile-time that the element only has a single child, it'd be preferable to implement Option 2 (although that incurs more implementation complexity). Interested in thoughts though!

@lessp
Copy link
Member

lessp commented May 11, 2019

I like the idea of having styles more declarative and most of the time we abstract things away like this during my day-to-day using React, either by making or own abstractions and/or with the help of something like: https://styled-system.com/api/.

But I wonder if it'd be hard to cover every corner-case which can be solved with a style-properties way and that it perhaps would be easier to provide more Primitives and leave abstractions up to the developer? 🤔

One question that comes up is how to handle pseudo/state-styles? Would every Primitive require an extra set of properties for e.g. hover and active? Or are there other ideas?

<Container 
  activeColor=Colors.green
  color=Colors.red 
  hoverColor=Colors.blue
>
...
</Container>

Nonetheless it's a cool direction to explore and points to one of the big benefits when using Flutter which enables you to move very fast using their provided UI-abstractions!

Following! 🙂

@manu-unter
Copy link
Collaborator

manu-unter commented May 11, 2019

The multiple children problem is a real one - one we should definitely think through thoroughly.

Concerning option 1, only allowing a single child: I'm not sure if that would be a desirable or understandable solution. My current idea of JSX and html semantics is that you can always put multiple children inside one element and it should be clear what happens with that. There are a few cases where only certain element types are allowed, but I would even consider those bad design decisions. I don't know of any existing limitations to one element anywhere though. So I think we should avoid this solution.

About option two: making these primitives implicit containers with respect to some semantics
I think this is a better solution and it behaves more consistently with how existing layout models work. I'm not sure about the behavior of things like <BoxShadow> though. Assuming I have two children inside a <BoxShadow>, and they don't assemble to a single rectangle due to differences in their dimensions. Would the shadow be applied to each of the children or to their bounding box?
My mental model of element composition tells me that it should be the bounding box, even though that is probably never the desired outcome. Applying it to each of the children also feels wrong though, since a single <BoxShadow> element would end up creating two box shadows.
The same questions apply for padding or other things, in my opinion. The problem seems to be that a primitive whose naming implies that it only does one thing - padding, shadow, whatever - in fact also ends up a layout container.

What I'm getting at with this is a third idea:
We could create a different layer of abstraction that might be more explicit about the layout container behavior and then offer features like padding, border, box shadow etc. as props on that component. Let's say we call it a <Box>.

This would be a middle ground between the fine grained widgets from flutter and the one stop shop style prop from css. I think it would be the best abstraction for the JSX world.

About the hover, active, disabled etc states - I'm unsure if we should stick that closely to web concepts here. In react, if this weren't part of the DOM, one would typically model these using a custom const isHovered = useHoverState(myElementRef) hook and then adapt the render result based on that state. This is a much more powerful approach and it doesn't require additional platform features that one would need to know. What do you think about going in a similar direction?

@wokalski
Copy link
Collaborator

briskml/brisk-reconciler#46 will create some really good facilities to implement it in a type safe manner.

@glennsl
Copy link
Member

glennsl commented Oct 28, 2019

I really like the idea of layout components, but still a bit uncertain about "decoration" components. I'd love to play around with a prototype though.

Re: multiple children for BoxShadow

Restricting to a single child with a runtime error seems less than great (for obvious reasons I hope). It could perhaps be a warning though, if the behavior in the case of multiple children is either degraded or unreasonably expensive.

Option 2, the bounding box, also doesn't seem all that reasonable to me, assuming the bounding box is rectangular, as "boxes" typically are. It seems somewhat reasonable for a border, but I would expect a BoxShadow (which is also "boxy" by name, although I think that's mostly for historical reasons) to shade the clip area, not the bounding box.

However, even in the case of a single child, it should use the clip are, not the bounding box. And even for borders, it should be possible to create rounded and even circular/elliptical borders. If not using the clip area for this it certainly needs to add to the clip area.

Does it matter whether there's one or more children, then, if it's based on clip area?

(Implicit) Inheritance of "style" properties

It would be very convenient to have certain properties inherited, the font properties in particular. Otherwise, in order to consistently render text, you ought to pass around all these properties: font-family, font-size, font-style, font-weight, line-height, and possibly font-variant, font-kerning, font-stretch, word-spacing and letter-spacing. These could of course be gathered into a single value, but you need some way to selectively override some of these properties relatively conveniently.

Additionally, in design work, you usually don't work with absolutes. Like with layout, which you specify relative to the current container, not to the whole. You say "this container should be split into three equal parts", not "this container should have three parts that are a fifth the size of the window it's in".

The same applies to many style properties. Most trivially, line-height is of course typically based on the size of the text that should occupy the lines, but also paddings and margins, border widths and even the size of adjacent elements need to be sized relative to each other. That means they need some common reference, which is usually the font size. em and ex are units defined relative to font-size. But their convenience also rely heavily on font-size being implicit/inherited.

@manu-unter
Copy link
Collaborator

This is a very relevant summary of the issues we currently have within react itself around element composition and interaction between the elements.
https://gist.github.com/ryanflorence/10e9387f633f9d2e6f444a9bddaabf6e
Some of them also apply to revery, and I think we should do it best to make sure we offer better solutions to this than react does while we can do so without major disturbance in the ecosystem

@glennsl glennsl added A-components Area: Consumer-exposed components providing some convenience over the primitives A-layout Area: Layout A-primitives Area: Primitives, the basic building blocks A-rendering Area: Rendering artifacts, features etc. A-technical Area: Technical issues not directly related to features labels Nov 25, 2019
@lessp
Copy link
Member

lessp commented Mar 12, 2020

Any more findings here? 🙂 I think trying to settle on a form would be a big step forward in opening up for a lot of other UI-work such as revamping and creating base components among other things!

As @glennsl was saying I think the biggest issue with the current compositional components is that they don't let its style props through. Like this (IIRC):

<Row>
  <Padding>
    ...

(children of Padding are no longer in a row)

@lessp
Copy link
Member

lessp commented Mar 31, 2020

Might be relevant and/or helpful:

https://open-ui.org/

bryphe added a commit that referenced this issue Sep 23, 2020
The `opacity` style option was removed in #540 , in trying to factor to semantic components. However, since we don't have that yet (dependent on some larger refactorings, as discussed in #489 ) - we should bring this back.

I'd prefer to use a more semantic component like `<Opacity />`, however, in the current design, that can impact layout, as every primitive has an associated layout node.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-components Area: Consumer-exposed components providing some convenience over the primitives A-layout Area: Layout A-primitives Area: Primitives, the basic building blocks A-rendering Area: Rendering artifacts, features etc. A-technical Area: Technical issues not directly related to features enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants