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

New Anchoring Logic for Advanced Use Cases #44

Merged

Conversation

creativecreatorormaybenot
Copy link
Contributor

Description

Hi @rrousselGit, this PR accomplishes a slight refactor of the flutter_portal package to allow for some more advanced and improved use cases that were previously impossible to achieve.

Here is an example video of one of the use cases:

Ztu-7RVFp4yj0mr8-Ztu-7RVFp4yj0mr8.mp4

Refactoring

There are basically two parts to the refactor:

  1. The introduction of the abstract Anchor class.
  2. Renaming / simplification of existing concepts.

The idea behind the Anchor class is to provide an abstract interface that is disconnected from the actual layout implementation to take care of anchoring a portal follower to its target.
There are default implementations, but in order to make the parameters and outputs of the anchors understandable, we felt like it was necessary to align the naming everywhere.

Non-fragility

The new logic should be super solid because it makes use of a custom link implementation that can handle the offsets in a way that essentially perfectly integrates with the existing LayerLink concepts and the framework. This means that there are no hacky workaround or additional layout / painting calls to make this work but the offsets are directly extracted from the link.

Contributing

We realize that you @rrousselGit do not have time to maintain this package at this time. That is why I completed the full refactor already instead of pushing that work onto you.

If you can give generally share your thoughts on whether and how this can be integrated into the repo / package, that would be extremely helpful, hopefully for everyone 🙏

@fzyzcjy
Copy link
Owner

fzyzcjy commented Feb 28, 2022

@rrousselGit Ok :)

The only thing you need me for I believe is publishing, until we sort out rights for it.

Btw @tkfinitefield has contacted me indeed in January using email, but I seldom check that mailbox so I missed it :( Only a few days ago did I see and reply to his email, as well as pub.dev's invitation link (which is already expired).

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 1, 2022

Hi, this does not seem to work on flutter 2.10?

Please have a look at: #13 (comment)

@creativecreatorormaybenot
Copy link
Contributor Author

@fzyzcjy Hi, that is right - there was a commit on stable that made the leader private. However,.this has been fixed.again later. On master, it works. Maybe we want to rollback until the next stable version 🤔

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 1, 2022

Interesting... Could you please provide a bit more details about this commit and fixes (sounds like there should exist some PR with discussions about this?)

A naive thought: Can we copy-and-paste the LayerLink into this repository and modify that field?

Maybe we want to rollback until the next stable version

That may be the final workaround :( But then we need to wait for about 3 months...

@creativecreatorormaybenot
Copy link
Contributor Author

@fzyzcjy you can follow the links in flutter/flutter#96486

@creativecreatorormaybenot
Copy link
Contributor Author

@fzyzcjy we can request a cherry pick for that PR. Then this could go to stable in the next days / weeks 🚀

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 1, 2022

@creativecreatorormaybenot Sounds great!

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 7, 2022

@creativecreatorormaybenot Hi, that PR in Flutter repo is P6, which means "unlikely to proceed". Thus maybe we should do something else.

I tried to copy Flutter's original LayerLink together with its friends who need using its private methods. PR is at: #47

However, it is failing. Could you please have a look at it? Because the failed tests seem to be originated from this PR.

fzyzcjy added a commit that referenced this pull request Mar 7, 2022
@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 7, 2022

Btw I am wondering why PortalEntry is renamed to PortalTarget? This sounds like a breaking change :(

@creativecreatorormaybenot
Copy link
Contributor Author

@fzyzcjy yes - this was part of Refactoring 2..

The concepts throughout the package were somewhat off, so we felt the need to simplify / unify them.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 7, 2022

@creativecreatorormaybenot Agree! Looking forward to your further refactoring!

@venkatd
Copy link
Contributor

venkatd commented Mar 7, 2022

@fzyzcjy do you mean it may take some time before LayerLink is made public again on the Flutter stable channel?

Yes as @creativecreatorormaybenot said, it was easy to mix up the meaning of some of names before. So the words "follower" and "target" were meant to clarify that the follower widget follows the target widget by the layout rules of an Anchor.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 8, 2022

@venkatd

@fzyzcjy do you mean it may take some time before LayerLink is made public again on the Flutter stable channel?

I guess 3 months

Yes as @creativecreatorormaybenot said, it was easy to mix up the meaning of some of names before. So the words "follower" and "target" were meant to clarify that the follower widget follows the target widget by the layout rules of an Anchor.

I see. Doc may be needed for new comers :)

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.

4 participants