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

Rework Scale System #147

Closed
wants to merge 13 commits into from

Conversation

caseycrogers
Copy link
Contributor

@caseycrogers caseycrogers commented Dec 27, 2021

📜 Description

This PR completely reworks the feedback_widget to use CustomMultiChildLayout instead of Stack.

💡 Motivation and Context

This has a number of advantages:

  1. The layout of each of the three key widgets (controls, sheet, screenshot) relative to the others is explicit and reactive. Previously, depending on the user device and orientation, the widgets could end up overlapping and/or looking awkward.
  2. It becomes much easier to reason about and modify the widgets' layout.
  3. All the elements animate in sync with each other.

I was especially finding the existing system to be too unmanageable while trying to update the bottom sheet to be a draggable sheet.

TODO:

  1. Don't bug out when the keyboard is shown ✔️
  2. Tweak the layout and incoming animation ✔️
  3. Make the controls column wrap if it is too long for the screen (particularly relevant in landscape mode) ✔️
  4. Expose sheetFraction in FeedbackThemeData? ❔

💚 How did you test it?

Goofed off with the example widget. Soon to add this to my existing app via a path dependency.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@caseycrogers caseycrogers requested a review from ueman as a code owner December 27, 2021 05:06
@caseycrogers caseycrogers marked this pull request as draft December 27, 2021 05:07
@caseycrogers caseycrogers changed the title Rework Scale System [WIP] Rework Scale System Dec 27, 2021
Copy link
Owner

@ueman ueman left a comment

Choose a reason for hiding this comment

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

Hey, thank you very much for this.

Previously, depending on the user device and orientation, the widgets could end up overlapping and/or looking awkward.

Yeah, I agree. That's still from when I started with Flutter 😅

@caseycrogers
Copy link
Contributor Author

caseycrogers commented Dec 28, 2021

@ueman The PR is mostly done. I've also made the controls column wrap if it's too tall to fit.

I have a couple question around the sheet size if you have a minute:

  1. What should the default height of the sheet be? I've picked 25% of the screen because it was close to the original.
  2. What should we do about the sheet in landscape mode? The screenshot is pretty small, but the sheet is also unusably small. Rock and hard place.
  3. Should we let developers customize the sheet's height via FeedbackTheme? Should the parameter be a fraction of screen size, or number of pixels? Both and the framework will pick the min or max between the two?

@ueman
Copy link
Owner

ueman commented Dec 28, 2021

I've also made the controls column wrap if it's too tall to fit.

Awesome.

  1. What should the default height of the sheet be? I've picked 25% of the screen because it was close to the original.

That's fine for me.

  1. What should we do about the sheet in landscape mode? The screenshot is pretty small, but the sheet is also unusably small. Rock and hard place.

Maybe in landscape it shouldn't be a bottom sheet but layout where both things are next to each other? That's probably more sensible because we're having much more horizontal space than vertical. That's probably work for another PR, though. For now, leaving it as it is fine as long as it's not worse than before.

  1. Should we let developers customize the sheet's height via FeedbackTheme? Should the parameter be a fraction of screen size, or number of pixels? Both and the framework will pick the min or max between the two?

I think I prefer having a fraction available in the FeedbackTheme. It seems reasonable. The docs for it should probably say that the recommended height of the sheet is around a quarter to a third.

If you're having trouble with the golden images again, just ignore them and I'll fix it them you want.

@caseycrogers
Copy link
Contributor Author

All done! I left the golden images and changelog/version bumping to you.

The changes to support a draggable bottom sheet are near done too so if you want to do one update, I could get the bottom sheet done pretty quick once this is merged.

Let me know if you think this PR needs any changes!

@caseycrogers caseycrogers changed the title [WIP] Rework Scale System Rework Scale System Dec 28, 2021
@caseycrogers caseycrogers marked this pull request as ready for review December 28, 2021 20:08
@ueman
Copy link
Owner

ueman commented Dec 31, 2021

I'm going to close this, as the changes are also included in #149

@ueman ueman closed this Dec 31, 2021
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.

2 participants