Skip to content

Conversation

@subsymbolic
Copy link
Contributor

Task/Issue URL: https://app.asana.com/0/361428290920652/688515420618259
Tech Design URL: N/A

Description:
Add a native feedback form for general and broken sites feedback.

Note: the UI uses native text fields, I will be validating that with design and expect to do a final pass to polish.

Steps to test this PR:
TEST 1: Broken site:

  1. From the browser page, select the "Report Broken Site"
  2. Note that the url is filled in and submit button active
  3. Submit the feedback
  4. Ensure it appears in https://app.asana.com/0/569473074191537/717268129349527
  5. Check the url and device details in the Asana task are correct
  6. Now delete it from Asana!

TEST 2: Broken site validation:

  1. From the browser page, select the "Report Broken Site"
  2. Note that the url is filled in and submit button active
  3. Delete the url
  4. Note that the submit button is now inactive
    Note: we only do basic "empty" validation here (the same as extensions) as a user may just want to enter text e.g "facebook"

TEST 3: Feedback Message:

  1. From the settings page, select "Leave Feedback"
  2. Note that the submit button is inactive
  3. Enter message text
  4. Submit the feedback
  5. Ensure it appears in https://app.asana.com/0/413877547933099/717275776651708
  6. Check the message and device details in the Asana task are correct
  7. Now delete it from Asana!

Internal references:

Software Engineering Expectations
Technical Design Template

@subsymbolic subsymbolic requested review from CDRussell and brindy June 20, 2018 13:30
<string name="feedbackModalDescription">Anonymously share your feedback to help us improve the DuckDuckGo Privacy Browser</string>
<string name="feedbackReportBrokenSite">Report a broken site</string>
<string name="feedbackDomainAdded">Domain Added to Feedback Report</string>
<string name="feedbackUrlHint">Enter url</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

super-picky, but should URL be in uppercase?

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

Tested it and it worked. Code LGTM - just a couple of minor picky comments :)

}

val viewState: MutableLiveData<ViewState> = MutableLiveData()
private val viewValue: ViewState get() = viewState.value!!
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a personal thing, but I tend to group variables with similar access modifiers together, with private nearer to the constructor, if there is one. I don't know if there's a convention for this in Kotlin though?

@subsymbolic
Copy link
Contributor Author

@brindy I made some small final copy and layout changes, feel free to rereview.

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

LGTM

@subsymbolic subsymbolic merged commit 4831e05 into develop Jun 21, 2018
@subsymbolic subsymbolic deleted the feature/feedback branch June 21, 2018 12:33
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