Skip to content

Expose the leader field of the LayerLink class. #96451

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

Closed
wants to merge 2 commits into from

Conversation

cchamm
Copy link

@cchamm cchamm commented Jan 11, 2022

Some packages, like local_hero, need to access LayerLink.leader in order to work. LeaderLayer? get leader => _leader is added to the LayerLink class again.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 11, 2022
@goderbauer
Copy link
Member

I took another look at this and I believe we can simplify the LeaderLayer even further, which will also allow us to bring back the LayerLink.leader property: #92598.

@goderbauer goderbauer self-requested a review January 12, 2022 00:34
@dnfield
Copy link
Contributor

dnfield commented Jan 12, 2022

local_hero may want to consider adding itself to flutter/tests so this doesn't get broken again

@goderbauer
Copy link
Member

goderbauer commented Jan 12, 2022

Closing this in favor of #92598 #96486, which has been merged.

@goderbauer goderbauer closed this Jan 12, 2022
@cchamm
Copy link
Author

cchamm commented Jan 13, 2022

local_hero may want to consider adding itself to flutter/tests so this doesn't get broken again

Adding test to packages/flutter/test/rendering/layers_test.dart, right?

@goderbauer
Copy link
Member

No, that will not help :). Packages can contribute their tests to https://github.com/flutter/tests as explained in the readme over there: https://github.com/flutter/tests/blob/master/README.md. That gives us a signal whether our changes break anything in the real world and if it does we will either reconsider the change or provide a breaking change announcement with migration guide.

@creativecreatorormaybenot
Copy link
Contributor

Closing this in favor of #92598, which has been merged.

@goderbauer I do not understand - #92598 was merged last year and this PR was opened 17 days ago - how can this have been closed in favor of #92598 when this PR is only a follow-up?

Is there anything wrong with this change ~

@goderbauer
Copy link
Member

I accidentally linked to the wrong PR in that comment. Meant to link to #96486, which re-exposes the leader field in a cleaner way.

@creativecreatorormaybenot
Copy link
Contributor

@goderbauer thanks! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants