Skip to content

Abstract flexbox over LayoutTree (rebased on top of latest main)#238

Merged
alice-i-cecile merged 13 commits intoDioxusLabs:mainfrom
nicoburns:abstract-over-storage-merged
Oct 24, 2022
Merged

Abstract flexbox over LayoutTree (rebased on top of latest main)#238
alice-i-cecile merged 13 commits intoDioxusLabs:mainfrom
nicoburns:abstract-over-storage-merged

Conversation

@nicoburns
Copy link
Copy Markdown
Collaborator

@nicoburns nicoburns commented Oct 13, 2022

This PR is #202 rebased onto the history rewrite, and with the latest main merged in to resolve conflicts.

Files other than flexbox.rs were merged automatically by git.
Flexbox.rs was taken from this branch, and the following commits were manually ported in:

Original PR description below:


This PR makes the flexbox algorithm generic over a LayoutTree trait.

At the time of filing this PR, the LayoutTree trait mostly abstracts over the broadest set of functions that the Taffy struct satisfies. I don't know or think this is the best and most general solution to flexboxing: Taffy internally does caching of calculations that a user's LayoutTree shouldn't really be concerned about. Perhaps some of the state - like caching - could remain in a Taffy struct, but this level of specificity intrinsically ties a user's UI system to Taffy due to dependent data structures.

Generally, since Taffy uses its own types, most implementations will be tied to Taffy anyways, so perhaps this linking of user code to Taffy is not a big deal.

Also - this trait supplies a method that returns a FlexboxLayout (not the final layout, despite the confusing name) which might not be exactly a trait implementation would return the "current state" of the DOM Node. For instance, in Dioxus, we are using the CSS Cascade to supply the "current state" of the DOM node's style. I think before publishing out these changes, it might be worth further abstracting the trait to supply single-level Taffy attributes. Perhaps a "TaffyAttribute" enum could be supplied so use cases like Dioxus' cascading CSS could be implemented efficiently.

Closes #182

@nicoburns nicoburns mentioned this pull request Oct 13, 2022
@nicoburns nicoburns marked this pull request as ready for review October 20, 2022 18:59
@nicoburns
Copy link
Copy Markdown
Collaborator Author

@alice-i-cecile What would it take to get this PR merged?
(note: this PR is not my work. I just cleaned up the git conflicts because I would like to build on top of it)

While I don't think the abstraction has quite been perfected yet, I'd rather do further work on top of this (and make further improvements in follow up PRs), because I think it's much cleaner (key improvement for me is passing the storage in an explicit parameter hidden behind a trait, rather than using self). However, I'm also reluctant to build too much on top of this as:

  • They are then also blocked on this being merged
  • It's quite a big refactor and it seems to generate not-particularly-easy-to-resolve conflicts every time other PRs are merged.

@alice-i-cecile
Copy link
Copy Markdown
Collaborator

I need to dedicate an hour or two to review this, but I similarly feel that this should be merged pretty aggressively.

I'll try to get that done for you by end of day next Monday (my 20% time at work). May happen before that, but you can hold me to that deadline.

Copy link
Copy Markdown
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'm on board. This is well done, and in-line with my broader vision for how this library should evolve to support multiple layouts.

Merging now.

@alice-i-cecile alice-i-cecile enabled auto-merge (squash) October 24, 2022 20:48
@alice-i-cecile alice-i-cecile merged commit 9118437 into DioxusLabs:main Oct 24, 2022
@nicoburns
Copy link
Copy Markdown
Collaborator Author

Just wanted to say thanks for reviewing this :)

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.

Introduce some tree abstraction

3 participants