Support Custom Feedback Prompts and Data Types #78

caseycrogers · 2021-05-11T22:10:13Z

📜 Description

Allows users to customize the contents of FeedbackBottomSheet so that they can prompt users for feedback more complex than a single String. This also enables them to freely stylize the bottom sheet beyond the options in FeedbackTheme.

The biggest change is the addition of a getFeedback argument to BetterFeedback. This is where users provide their custom widgets. It defaults to getStringFeedback which mirrors the existing BetterFeedback implementation.

THIS IS A BREAKING CHANGE. Specifically, the feedback argument of OnSubmit is now an Object rather than a String to accommodate custom feedback types. Existing users will get a compile time error and will have to add as String to any reference to feedback in BetterFeedback.of(context).show((feedback) { ... }).

Alternatives Considered:

Using type parameters instead of Object for feedback
Because you can't have a default type, this would require splitting BetterFeedback into two separate classes, one for default behavior with String feedback, and one with custom behavior and parameterized feedback.
This became exceedingly messy and bug prone, especially for writing tests and the sample app.
A Feedback class that contains an Object content member field
This didn't seem to provide any clear advantages over feedback being an object.

💡 Motivation and Context

See #77.

💚 How did you test it?

TBD

📝 Checklist

🔮 Next steps

Update changelog
Update golden images

…lled' is ignored, all else pass

caseycrogers · 2021-05-11T22:12:59Z

@ueman This is still WIP. It's buggy and I haven't added any tests for the new logic yet. However, while I'm fixing it and adding tests, if you have any time to glance over the high level approach I've taken I'd love to hear if you have any feedback!

Also, the golden images are failing in a trivial manner-some elements that had one pixel borders now don't and some images that didn't have one pixel borders now do. I assume this is a change to Flutter? But will await your input before making any updates to the golden images.

…r.of

caseycrogers · 2021-05-11T22:31:28Z

test/feedback_test.dart

@@ -123,7 +129,7 @@ void main() {
        await tester.tap(submitFeedbackButton);
        await tester.pumpAndSettle();
      });
-    }, skip: true);


A bug was slipping through the tests because this one was being skipped. I unskipped it and added await tester.pumpAndSettle(); to fix the bug in the test itself that was (presumably) why it was being skipped.

ueman · 2021-05-12T07:05:53Z

I'm not sure if this is the right approach to the problem.

Why not just include a Dropdown with options for Bug, Feature Request, Other by default and have the option of toggling its visibility?
That way we could also provide default translations.

However I really appreciate your PR. Having a categorization seems like a great addition.

ueman · 2021-05-12T07:10:02Z

Also, the golden images are failing in a trivial manner-some elements that had one pixel borders now don't and some images that didn't have one pixel borders now do.

They need to be created/tested on a macOS system. They're a little bit different on Windows/Linux. I should really document that somewhere 😅

caseycrogers · 2021-05-12T17:12:38Z

I'm not sure if this is the right approach to the problem.

Why not just include a Dropdown with options for Bug, Feature Request, Other by default and have the option of toggling its visibility?
That way we could also provide default translations.

However I really appreciate your PR. Having a categorization seems like a great addition.

My reasoning for letting users provide arbitrary widgets (as opposed to just providing them with a prebuilt drop down and configuration) is I think that the set of features rapidly becomes too expansive to realistically build and provide configuration for every one. For example, here's just with my use case:

I want to grey out submit until the drop down has been selected
My app is a translation dictionary so my drop downs would be Bug, Translation Error and other
The existing text field feels like it implies a short amount of input. I want to replace it with a taller textfield (one thing I'm realizing is the existing bottom sheet layout doesn't provide a lot of room-perhaps as a follow up we could add a feature that allows the bottom sheet to expand up and cover the screenshot when it's active)

Providing the option for custom bottom sheets means any time a user has a request that's too niche to add to the base API, they can just build it themselves for their project.

That said, the custom widget solution does have downsides:

Requires the user to write a lot of code even if they want something only slightly different from the default
Propagates a type parameter through the entire code base making it a bit harder to read and more bug-prone
Because Dart doesn't support optional type parameters (Problem: Adding a 2nd (or later) type parameter to a class breaks clients dart-lang/language#283) I had to provide backwards compatibility via two classes instead of just adding a new argument with a default value. This makes the new feature not very discoverable

Anyway, if this doesn't convince you no hard feelings! I'll just use a local copy of this fork in my project.

ueman · 2021-05-12T17:44:07Z

Ah okay I see, thanks for your elaborate answer.

I think you could avoid a lot of those generics by just making a UserFeedback class which has the text and screenshot as fields. In your custom bottom sheet builder you can return a subclass. In the feedback handler you then just cast it to its subclass.
Or by adding an extra field UserFeedback which can hold arbitrary information.

Though it's not necessarily a better approach.

A solution for the bottom sheet being too small would be a two step submit process.

First getting the annotated screen and then select type and input a message.

caseycrogers · 2021-05-12T19:37:31Z

Ah okay I see, thanks for your elaborate answer.

I think you could avoid a lot of those generics by just making a UserFeedback class which has the text and screenshot as fields. In your custom bottom sheet builder you can return a subclass. In the feedback handler you then just cast it to its subclass.
Or by adding an extra field UserFeedback which can hold arbitrary information.

Though it's not necessarily a better approach.

A solution for the bottom sheet being too small would be a two step submit process.

First getting the annotated screen and then select type and input a message.

Yeah, either a subclass or Object that the user then has to cast would make the rest of the code cleaner. The Flutter routing system uses this for RouteSettings.arguments. I went with generics because while they make the code base harder to follow, they make the user experience cleaner (outside of nescessitating two classes) and type-safe. Down the line Dart may even add support for default types for generics which would allow us to merge the two classes back into one and retain the generic typing. The issue I linked above has a decent amount of attention on it.

Another option is to merge the classes back together and retain the generics but force the user to specify T as String and cast the default value. This is really gross though because it would make any call to BetterFeedback() without getFeedback or T specified a runtime error.

In the end it's a value judgement-let me know which solution you prefer and I can update the PR to follow it!

For the bottom sheet, are you comfortable leaving that to a follow up PR? Perhaps we could publish the two PRs at the same time as a custom bottom sheet may not be every helpful if your bottom sheet is extremely size constrained.

…o custom_form

caseycrogers · 2021-05-20T00:23:24Z

Using generics with two separate classes proved prohibitively messy in tests and the example app-I've moved to using Object for the feedback data type.

This is non-ideal as it requires users to cast feedback to their desired type in BetterFeedback.of(context).show(...) but at least now there's only a single class that has a getFeedback argument that defaults to getStringFeedback.

…he working test.

…r.of

caseycrogers · 2021-05-20T21:40:46Z

example/lib/main.dart

@@ -112,16 +131,16 @@ class MyHomePage extends StatelessWidget {
              ElevatedButton(
                child: const Text('Provide feedback'),
                onPressed: () {
-                  BetterFeedback.of(context)?.show(
+                  BetterFeedback.of(context)!.show(


imo ! is preferable as it'll fail loudly instead of silently (ran into a few bugs that I would have identified faster with !)

Yeah, that's definitly something I want to tackle, along with making the image not nullable.
In the past I wasn't comfortable making it non nullable but now it's quite popular and nobody complained that it fails.

caseycrogers · 2021-05-20T22:10:23Z

example/lib/main.dart

@@ -179,6 +198,19 @@ class MyHomePage extends StatelessWidget {
          ),
        ),
      ),
+      floatingActionButton: MaterialButton(


A floating action button could look a little sloppy, but I figured this should be fine?

caseycrogers · 2021-05-20T22:11:16Z

example/lib/main.dart

+      child: MaterialApp(
+        title: 'Feedback Demo',
+        theme: ThemeData(
+          primarySwatch: _useCustomFeedback ? Colors.green : Colors.blue,


Change colors to emphasize current feedback mode

caseycrogers · 2021-05-20T22:13:24Z

lib/src/feedback_bottom_sheet.dart

+    // Typically, the navigator would be provided by a `MaterialApp`, but
+    // `BetterFeedback` is used above `MaterialApp` in the widget tree so that
+    // the nested navigation in navigate mode works properly.
+    return Navigator(


This used to be an Overlay. I switched to Navigator as DropDownButton and several other material widgets require a navigator-not just an overlay (all navigators also provide an overlay).

Interesting. At first look this seems like the better approach.

Does that impact the navigator of your *App?
In the past I had problems with overlays not being at the correct position because I used a MaterialApp inside this library. MaterialApp also provides a Navigator.

An simple test for that would be to show a dialog and open the feedback view iirc.

I'll hope that this change does not reintroduce that bug.

Ah it probably doesn't because it's just used in the FeedbackBottomSheet.

From messing around manually with the sample app this worked well. I'm not quite sure what the right test to verify this is-but if you have ideas for test scenarios to ensure proper behavior I can try writing them up.

One thing that could be useful: A test that opens the feedback in navigate mode, clicks a couple buttons, and then hits back a couple times and verifies that the appropriate widgets are present between each action.

caseycrogers · 2021-05-20T22:14:28Z

lib/src/feedback_bottom_sheet.dart

-                  },
-                ),
-              ],
+          child: SingleChildScrollView(


I added a scroll view here to prevent overflow, but maybe we should leave this up to users? Generally, I tried to make getFeedback easy to write but slightly less customizable.

caseycrogers · 2021-05-20T22:15:13Z

lib/src/feedback_bottom_sheet.dart

-              ],
+          child: SingleChildScrollView(
+            child: Container(
+              padding: const EdgeInsets.all(30),


This could also be pushed into getFeedback so if users wanted to customize padding they could do so there.

Or, probably better, make this configurable via FeedbackTheme?

caseycrogers · 2021-05-20T22:16:09Z

lib/src/feedback_widget.dart

+                      // height should be screen size minus the bottom edge of
+                      // screenshot widget:
+                      //   1 - (scaleOrigin + height*scaleFactor)
+                      (1 - (.35 / 2 + 1.65 / 2 * .65)),


This one's a doozy, but it ensures that the bottom sheet is programmatically positioned relative to the screenshot.

caseycrogers · 2021-05-20T22:17:44Z

@ueman This one ended up being pretty big! Let me know what you think when you have the chance to look at it. Happy to make any changes you see necessary.

ueman

I definitly need to check this out and see it in action.

ueman · 2021-05-21T10:57:44Z

lib/src/better_feedback.dart

+
+/// A function that returns a Widget that prompts the user for feedback and
+/// calls [OnSubmit] when the user wants to submit their feedback.
+typedef GetFeedback = Widget Function(OnSubmit);


I think this should also supply a context to the user, because it's kinda like a Builder

Yeah I thought about that and went without it as users can just use a builder if they want access to a BuildContext-it'll just pollute the function arguments otherwise. That said, it's pretty conventional in Flutter to pass a BuildContext in this kind of situation.

How about I rename GetFeedback to FeedbackBuilder and add a BuildContext argument?

ueman · 2021-05-21T10:59:10Z

lib/src/feedback_widget.dart

-              if (widget.isFeedbackVisible)
+              // only display if feedback is visible or this widget is still
+              // animating out
+              if (widget.isFeedbackVisible || !animation.isDismissed)


This could probably be simplified to just if (!animation.isDismissed). In fact, you could replace bool isFeedbackVisible with bool get isFeedbackVisible => !animation.isDismissed (this will be slightly broader: it'll return true when the feedback is displayed AND while it's still animating out).

All that said, I think either works well and this way it's a little more obvious what the code is doing.

… submit button)

caseycrogers · 2021-05-21T18:32:13Z

I definitly need to check this out and see it in action.

I updated the sample app to have a toggle button! If you toggle it to custom mode it'll show an example with a moderately complex feedback form.

As is there's not a lot of space for feedback so for multipart forms you have to do a lot of scrolling. My idea would be in a follow up PR to make the height of the bottom sheet dependent on whether or not it's currently focused-so when its focused it slides up to take up ~90% of the screen (I might even be able to make it take up only as much space as is needed to fully display the feedback). You can go back to navigating/drawing by clicking on the top 10% to defocus it, or by swiping down on the top edge (think android or IOS system tray). This could be disabled or enabled via feedbackTheme as people with short feedback forms probably don't want the extra complexity.
We could also make feedback a two step process, but I think that's less desirable as it'll both take more work to develop and make it harder to go back and forth between updating the screenshot and writing the feedback.
I'll open a few issues for a set of improvements I've thought about doing (eg pressing back will close the feedback widget instead of navigating the screenshot) so we can organize the conversation around those-I'll write up any that you think are good ideas.

I'm using GetFeedback in my app already (via a local path dependency):

ueman

LGTM. Thanks!

I'm going to merge this via command line because I want to update the golden images first.

caseycrogers · 2021-05-24T02:50:59Z

Oh crap, I forgot to change getFeedback to feedbackBuilder. I think the latter does make way more sense-want me to send another PR your way before you release?

ueman · 2021-05-24T05:18:01Z

Oh crap, I forgot to change getFeedback to feedbackBuilder. I think the latter does make way more sense-want me to send another PR your way before you release?

I changed it 👍

First pass, golden test trivially failing, 'feedback callback gets ca…

0a26bf3

…lled' is ignored, all else pass

fixed bug in 'feedback callback gets called' and in FeedBackControlle…

082151d

…r.of

caseycrogers commented May 11, 2021

View reviewed changes

improved documentation

b0d7bc5

started updating example app

9a46a51

caseycrogers added 4 commits May 17, 2021 15:50

removed context argument from

15cadb7

updating example

98b86cc

Merge branch 'master' of https://github.com/caseycrogers/feedback int…

1cbd96e

…o custom_form

gave up on type generics and switched to for feedback type

6c3fcfc

fixed feedback callback test

6f8fc1a

caseycrogers mentioned this pull request May 20, 2021

Fixed 'feedback callback gets called' Test #81

Merged

5 tasks

caseycrogers added 12 commits May 19, 2021 22:11

removed unnecessary comment/timeout

75c3ab0

Rebased custom form on top of the callback test fix so I could have t…

39ebcf9

…he working test.

fixed bug in 'feedback callback gets called' and in FeedBackControlle…

2d50bc6

…r.of

improved documentation

4fc7b44

started updating example app

0562819

removed context argument from

862c0a5

updating example

82f3998

gave up on type generics and switched to for feedback type

b9e2a3d

attempting and failing to fix callback test

445f647

moved to object or something?

b98dec5

got lost while rebasing so I just muliganned feedback_test.dart

811db54

pulled from origin

21be31e

caseycrogers added 4 commits May 19, 2021 23:03

minimal custom feedback test is working

11fc1be

brought up to date with master

725401a

commit before refactoring entire feedback alignment

5d1e483

fixed bottom sheet alignment and finished example

3cd4f11

caseycrogers commented May 20, 2021

View reviewed changes

caseycrogers added 2 commits May 20, 2021 15:48

polished documentation

75e43f0

added documentation to custom_feedback

45d04b2

caseycrogers commented May 20, 2021

View reviewed changes

caseycrogers marked this pull request as ready for review May 20, 2021 22:17

caseycrogers requested a review from ueman as a code owner May 20, 2021 22:17

ueman reviewed May 21, 2021

View reviewed changes

move scrolling into so users can customize scroll behavior (eg freeze…

d57220b

… submit button)

caseycrogers added 3 commits May 21, 2021 12:46

removed outdated comment

0147f70

added navigation test

52a666b

verify that feedback is open so that test can't pass trivially

ca2706e

ueman self-assigned this May 23, 2021

ueman approved these changes May 23, 2021

View reviewed changes

ueman closed this May 23, 2021

caseycrogers deleted the custom_form branch May 24, 2021 02:51

Uh oh!

Support Custom Feedback Prompts and Data Types #78

Support Custom Feedback Prompts and Data Types #78

Uh oh!

Conversation

caseycrogers commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

caseycrogers commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ueman commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ueman commented May 12, 2021

Uh oh!

caseycrogers commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ueman commented May 12, 2021

Uh oh!

caseycrogers commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caseycrogers commented May 20, 2021

Uh oh!

caseycrogers May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caseycrogers May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caseycrogers May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caseycrogers commented May 20, 2021

Uh oh!

ueman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caseycrogers May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caseycrogers commented May 11, 2021 •

edited

Loading

caseycrogers commented May 11, 2021 •

edited

Loading

ueman commented May 12, 2021 •

edited

Loading

caseycrogers commented May 12, 2021 •

edited

Loading

caseycrogers commented May 12, 2021 •

edited

Loading

caseycrogers May 20, 2021 •

edited

Loading

caseycrogers May 20, 2021 •

edited

Loading

caseycrogers May 20, 2021 •

edited

Loading

caseycrogers May 21, 2021 •

edited

Loading

caseycrogers commented May 21, 2021 •

edited

Loading