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

Break ViewStyle into LayoutStyle and NodeStyle #974

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Conversation

rylin8
Copy link
Collaborator

@rylin8 rylin8 commented Apr 9, 2024

Split up the ViewStyle struct into LayoutStyle and NodeStyle. LayoutStyle has all fields necessary for performing layout. NodeStyle has everything else. This change is to prepare for the switch from serde to protobufs.

@rylin8 rylin8 force-pushed the wb/rylin/layout-style branch 3 times, most recently from 7d58d94 to a45e299 Compare April 10, 2024 03:45
Copy link

github-actions bot commented Apr 10, 2024

Snapshot diff report vs base branch: main
Last updated: Wed Apr 10 10:55:55 PDT 2024, Sha: e1d91ac
No differences detected

@iamralpht
Copy link
Collaborator

Looks like a good clean up, and helps moving the layout communication to protobuf by making the layout style explicit. I'm going to be offline for the rest of today and for tomorrow, so if it's possible, it would be nice to land #972 first.

I realized in working on some other animation bugs that I need to make a large change to SquooshAnimate -- currently it works by mutating the two trees (view tree with all animations at start state, view tree with all animations at end state) into a single tree that can be morphed between the start and end states (with different "controls" for different animations). This is flawed because both of the incoming trees are destroyed to make the morphable tree, and the morphable tree can't be laid out anymore.

It would be better to simply make the view trees immutable after creating them, and then have SquooshAnimate construct a third tree from the two input trees -- perhaps the third tree has backlinks to the before/after trees, so that those trees can have new layout passes done, and the morphable tree can simply read the new layout values?

Anyway, all of that is to say, I haven't started on serious deeper animation changes yet, but there will be a bigger refactor coming to SquooshAnimate, too.

…truct

Split up the ViewStyle struct into LayoutStyle and NodeStyle. LayoutStyle has all fields necessary for performing layout. NodeStyle has everything else. This change is to prepare for the switch from serde to protobufs.
@rylin8 rylin8 marked this pull request as ready for review April 10, 2024 19:00
@rylin8 rylin8 requested a review from yiqunw700 April 10, 2024 19:01
Copy link
Member

@timothyfroehlich timothyfroehlich left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward, all checks pass, no screenshot changes so I think it's good to go

@timothyfroehlich
Copy link
Member

timothyfroehlich commented Apr 10, 2024 via email

@rylin8 rylin8 added this pull request to the merge queue Apr 10, 2024
Merged via the queue into main with commit 0a86d43 Apr 10, 2024
24 checks passed
@rylin8 rylin8 deleted the wb/rylin/layout-style branch April 10, 2024 22:22
@timothyfroehlich timothyfroehlich added this to the 0.26 milestone Apr 17, 2024
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.

None yet

3 participants