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.

Sign up for GitHub

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

Jump to bottom

Expandable Bottom Sheet 2 Electric Boogaloo #149

Merged
merged 22 commits into from
Dec 31, 2021
Merged

Expandable Bottom Sheet 2 Electric Boogaloo #149

merged 22 commits into from
Dec 31, 2021

Conversation

caseycrogers
Copy link
Contributor

@caseycrogers caseycrogers commented Dec 29, 2021 •
edited
Loading

📜 Description

Replaces #89 as the latter was so out of date it was easier to start over than to merge.

I'm going to self-review this a bit, but I think it's ready to review and merge? It's dependent on #147 so that'll have to go in first.

💡 Motivation and Context

💚 How did you test it?

📝 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 29, 2021 04:16
@caseycrogers caseycrogers mentioned this pull request Dec 29, 2021
Closed
5 tasks
caseycrogers
caseycrogers commented Dec 29, 2021
View reviewed changes
feedback/lib/src/feedback_widget.dart
painterController.clear();
},
builder: (context, screenshotChild) {
return CustomMultiChildLayout(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section has gotten huge-it'd probably be a good candidate for breaking out into a separate widget. I could add that to this PR or we could put it off to a future PR.

Copy link
Owner

Choose a reason for hiding this comment

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

Your call

@caseycrogers caseycrogers mentioned this pull request Dec 29, 2021
Closed
ueman
ueman reviewed Dec 29, 2021
View reviewed changes
@ueman
Copy link
Owner

ueman commented Dec 29, 2021

I'll hopefully have some time over the next few days and test this more throughout. Thanks again!

@caseycrogers caseycrogers mentioned this pull request Dec 29, 2021
Closed
caseycrogers
caseycrogers commented Dec 29, 2021
View reviewed changes
Copy link
Contributor Author

@caseycrogers caseycrogers left a comment

Choose a reason for hiding this comment

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

Fixed all the comments!

@ueman
Copy link
Owner

ueman commented Dec 29, 2021

Dragging seems to be broken on desktop. I haven't figured out why, though. On mobile it's really awesome.

@caseycrogers
Copy link
Contributor Author

Hmm, let me see if I can figure it out on my end. On Windows?

@ueman
Copy link
Owner

ueman commented Dec 29, 2021 •
edited
Loading

Hmm, let me see if I can figure it out on my end. On Windows?

I've tried on a Mac, but Windows is probably also affected. Maybe also web?

Also, I've just discovered another error: If the bottom sheet covers the whole screen and if you're then scrolling the text input to the top out of screen, you'll get a Stack Overflow exception.

Also the bottom sheet needs to be closed before taking the screenshot, otherwise it doesn't work.

@caseycrogers
Copy link
Contributor Author

caseycrogers commented Dec 29, 2021 •
edited
Loading

Hmm, let me see if I can figure it out on my end. On Windows?

I've tried on a Mac, but Windows is probably also affected. Maybe also web?

Also, I've just discovered another error: If the bottom sheet covers the whole screen and if you're then scrolling the text input to the top out of screen, you'll get a Stack Overflow exception.

Also the bottom sheet needs to be closed before taking the screenshot, otherwise it doesn't work.

As for desktop, I tried on Windows. The error should also exist for web. What's happening is DraggableScrollableSheet only works for dragging (not scrolling) and you can only scroll on those platforms.
I could put a PR against Flutter to fix this, but I think it's a little more complicated:
depending on the user's scroll direction (depends on platform and settings-thanks Apple, there used to be one standard here and you ruined it as a part of a conspiracy to discourage people from buying third party mice) and whether or not they're using a mouse with a scroll wheel or two finger drag on a touchpad, the sheet expanding will feel really jarring and unintuitive.

Once the DraggableScrollableController PR makes it into production Flutter (I think in about a month?) we could make the drag handle draggable on those platforms so even if scrolling won't expand the sheet, clicking and dragging on the handle would.

In the meantime, I propose the following: only allow the draggable sheet on mobile. We add a check somewhere where if it detects that the platform is not mobile and sheetIsDraggable is true, it'll print a warning to console and treat sheeIsDraggable as false. We can also make the default value platform dependent.

I'll look into the stack overflow and the screenshot issue now.

@caseycrogers
Copy link
Contributor Author

All the errors should be resolved. Sorry this was initially so buggy, I was lot more thorough this round.

The golden images are pretty badly garbled, I'm not sure what's going on there. Do you think you could take a crack at it?

@ueman ueman mentioned this pull request Dec 30, 2021
Closed
ueman
ueman reviewed Dec 31, 2021
View reviewed changes
feedback_gitlab/example/example.dart
@@ -2,7 +2,7 @@ import 'package:feedback_gitlab/feedback_gitlab.dart';
import 'package:flutter/material.dart';

void main() {
runApp(const BetterFeedback(child: MyApp()));
Copy link
Owner

Choose a reason for hiding this comment

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

This should be reverted

@ueman ueman mentioned this pull request Dec 31, 2021
Closed
5 tasks
@ueman ueman changed the base branch from master to feature/rework December 31, 2021 11:32
@ueman ueman merged commit bff7fdc into ueman:feature/rework Dec 31, 2021
@ueman
Copy link
Owner

ueman commented Dec 31, 2021

See #154 for what was wrong with the golden images

ueman pushed a commit that referenced this pull request Dec 31, 2021
@caseycrogers @ueman
Expandable Bottom Sheet 2 Electric Boogaloo (#149)
e8bb245 
* s/getFeedback/feedbackBuilder/

* initial version

* added background color back in

* half fix keybaord

* move padding logic outside of the layout

* make padding not comically small

* s/progress/animationProgress/

* center align controls and screenshot, fix keyboard behavior

* make controls wrap instead of overflow

* move sheet height into feedback theme

* half done

* remove import

* mostly done

* oh my god I think it's done

* responded to review comments

* fixed submit bug

* resolved submit and fade bugs

* fixed error on submit with keyboard up

* fixed stack overflow
@ueman ueman mentioned this pull request Dec 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ueman ueman ueman left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@caseycrogers @ueman