Skip to content
This repository was archived by the owner on Jun 13, 2024. It is now read-only.

Foldable support #677

Merged
merged 27 commits into from
May 17, 2022
Merged

Foldable support #677

merged 27 commits into from
May 17, 2022

Conversation

andreidiaconu
Copy link
Contributor

@andreidiaconu andreidiaconu commented May 5, 2022

Adds support for foldable devices (support newly added to Flutter), as shown in the following images. It also adds a new sample, related to foldable development. It showcases TwoPane, the same widget used in making the Gallery app foldable-aware.

Screenshot 2022-05-05 at 17 42 16

Screenshot 2022-05-05 at 17 42 55

Here is a screenshot of the TwoPane sample:

Screenshot 2022-05-09 at 11 51 59

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 the Flutter Style Guide recently, and have followed its advice.
  • 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.

@andreidiaconu
Copy link
Contributor Author

andreidiaconu commented May 9, 2022

Benchmark and golden tests are failing but I can't figure out why. Marking this as ready for review while I try to understand the error in the benchmark test.

LE: Benchmark seems to pass now.

@andreidiaconu andreidiaconu marked this pull request as ready for review May 9, 2022 08:54
Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

LGTM overall.

I wonder if it would be beneficial to s/dual/hinged/g everywhere. I feel like "hinged screen" is more common than "dual screen"

@andreidiaconu
Copy link
Contributor Author

andreidiaconu commented May 9, 2022

LGTM overall.

I wonder if it would be beneficial to s/dual/hinged/g everywhere. I feel like "hinged screen" is more common than "dual screen"

This works on all foldable devices, although the split in the middle of the layout happens on continuous screens only when the hinge is not flat. So for a Samsung Fold for example, the layout is not considered "foldable" if the screen is flat, but it then converts to a foldable layout when you start bringing the screens together (using it like a book).

I will go over the texts and naming to make sure this no longer points to "dual-screen" and points to "foldable" instead, as a general term.

@andreidiaconu
Copy link
Contributor Author

@guidezpl Made most of the changes you requested.

  • We still need to reach an agreement on the isDisplayDesktop changes.
  • There is new code in the dialogs demo that would benefit from a fresh review.

@andreidiaconu
Copy link
Contributor Author

@guidezpl Updated to latest master, with super constructor parameters. The only failing test is the Golden one. I could update the golden files, but I'm not sure if this requires a linux machine or if I can do it on a Windows or macOS myself.

@guidezpl
Copy link
Member

guidezpl commented May 17, 2022

Awesome, thanks! The updated goldens are available at https://github.com/flutter/gallery/actions/runs/2333115719. I have just a few small nits around language

Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

I think the new dialog option isn't best placed in the gallery, since it is primarily used on non-foldable devices, web especially. I would remove it and instead suggest adding the code as code snippets in the package API docs.

@andreidiaconu
Copy link
Contributor Author

andreidiaconu commented May 17, 2022

@guidezpl Not sure why goldens are still failing. It tries to create a new artifact but the failures folder does not exist. Seems to fail from unusual reasons. Do you know if this is happening on other PRs or branches? (If it is not related to the changes I made, a separate PR to address goldens failing might be a better idea than to fix here)

LE: Nevermind, I found the problem and it is seems to be related to changes I made.

Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

The goldens need to be updated for new description, I think, otherwise LGTM! Happy to merge this after the goldens issue is figured out

@guidezpl guidezpl merged commit 221023a into flutter:master May 17, 2022
@andreidiaconu
Copy link
Contributor Author

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants