Getting rid of containers

flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond

https://flutter.dev

BSD 3-Clause "New" or "Revised" License

162.18k stars 26.64k forks source link

Getting rid of containers #147432

Closed nate-thegrate closed 5 days ago

nate-thegrate commented 2 weeks ago

This pull request aims to make the Flutter framework more efficient, based on issue #147431.

Going into it, I didn't expect to spend 4 hours updating tests, but apparently find.byType(Container) is very convenient and was used a lot.

nate-thegrate commented 1 week ago

I'm going to try splitting this in half to make it easier to review. We'll see how it goes :)

auto-submit[bot] commented 1 week ago

auto label is removed for flutter/flutter/147432, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

jmagman commented 6 days ago

The Google testing failures look related to this PR:

Expected: exactly 2 matching candidates
  Actual: _TypeWidgetFinder:<Found 1 widget with type
"Container": [
            Container(bg: BoxDecoration(color: Color(0xffe8f0fe),
border: Border.all(BorderSide(color: Color(0x00000000))),
borderRadius: BorderRadius.circular(8.0))),
          ]>
   Which: is not enough

and another is failing at

final BuildContext context = tester.element(find.byType(Container));

The following StateError was thrown running a test:
Bad state: No element

@goderbauer could you help with the g3fix, or whatever would be needed to get those tests passing?

goderbauer commented 5 days ago

I am gonna rebase this to kick the checks again.

goderbauer commented 5 days ago

Actually, the github UI isn't allowing me to do a rebase. @nate-thegrate Can you rebase this with the latest main branch?

nate-thegrate commented 5 days ago

Done!

(Edit: that was a merge commit, not a rebase… I'm pretty sure autosubmit does a "squash and merge", so at least the main branch won't be as messy as this PR 🙂)

nate-thegrate commented 5 days ago

Merging per the recent Discord message:

christopherfujino: I am actively investigating autosubmit not working (in https://github.com/flutter/flutter/issues/148092 and a thread in this chat)—in the meanwhile, if:

all presubmits are green (including tree status)

PR has two approving Flutter GitHub org members (including the author if applicable)

Then Flutter GitHub org members should feel empowered to manually merge PRs, until https://github.com/flutter/flutter/issues/148092 is fixed.