Closed Bug 1079227 Opened 8 years ago Closed 8 years ago

Loop feedback form should always allow comments


(Hello (Loop) :: Client, defect, P2)



(Not tracked)

37.3 - 12 Jan
backlog Fx37+


(Reporter: RT, Assigned: standard8)



(Whiteboard: [rooms][uplift - minor string pick up in 37])

User Story

As a desktop client user I want to submit free text feedback if I select any of the existing options offered to me when I am unhappy about the experience.

Acceptance criteria:
* When I click the "Sad" Face I can select "Audio quality", "Video quality", Was disconnected", "Confusing" or "Other" and I can also select the free text field to add additional comments
* If I enter text in the free text field and I select another option, the free text field does not get cleared


(2 files, 3 obsolete files)

Attached image feedback form.png (obsolete) —
Per 1077257 we need a feedback form allowing users to provide details around any issues they may experience.
We should just leave the text box available all the time and submit its contents when submitting feedback even if the user doesn't select any category.
No longer blocks: 1077257
backlog: --- → -
This looks like a dup of bug 1086512
Closed: 8 years ago
Resolution: --- → DUPLICATE
Blocks: 1077257
I am re-opening this bug as bug 1086512 is about supporting the feedback form in rooms although this one is about changing the feedback form layout.
I added details in the user story field so requirements are clearer.
I also created bug 1113540 to track the link clicker UI implementation as I don't think there was a bug for this.
User Story: (updated)
Resolution: DUPLICATE → ---
Comment on attachment 8501020 [details]
feedback form.png

Marking as obsolete as we agreed to keep the "Other" field in another bug.
The UI will look similar to what we have currently implemented, just the behavior will change (ability to select the free text field with any option).
Attachment #8501020 - Attachment is obsolete: true
Maire can you confirm this can be marked as a Fx36 bug given the meta that was supposed to address this is a Fx36 bug?
Flags: needinfo?(mreavy)
I've marked for Fx36, P1, but I'll be moving it P2 later today when we normalize our priorities according to our new definitions.  See for more details.
backlog: - → Fx36+
Flags: needinfo?(mreavy)
Priority: -- → P1
Moving all P1->P2.  (P2 means a major bug that we very much want to fix, but we wouldn't stop ship or block the release for it.)
Here's the actual change from P1->P2 (per the previous comment).  P2 indicates a major bug, but not a stop ship.
Priority: P1 → P2
Assignee: nobody → standard8
Iteration: --- → 37.3 - 12 Jan
Points: --- → 2
Blocks: 1113540
Blocks: 1079216
Attached patch Improve the Loop feedback form. (obsolete) — Splinter Review
This does the necessary for what is required. I'll get a needinfo up in a moment for feedback on the actual functionality.
Summary: Desktop client UI needs a better feedback form → Loop feedback form should always allow comments
No longer blocks: 1113540
Romain/Sevaan, can you test the feedback form on our dev server and check you are both happy with the new form?

Things I've changed:

- Any option can now have text associated with it
- I've dropped the ':' from the end of "Other:" as it didn't seem right now that the text box applies to any category.

Example output on input dev server:

Note that to see the category annotation, you need to visit the "permalink"

To test, either:

- Set up a new profile, set the loop.server pref to "" (no trailing slash), restart FF, generate a room and visit, etc


- Ask me for a room link on irc.
Flags: needinfo?(sfranks)
Flags: needinfo?(rtestard)
I'm having a hard time getting it to work. I'm on the latest build and am using your updated pref in a new profile.

Still, I can see the screens and think we need to tweak the visuals a bit to fit with the rest of our screens. I'll file a bug for that.
Flags: needinfo?(sfranks)
Depends on: 1117941
Thanks Mark, it works fine for me on the standalone client although it has not been implemented in the  nightly desktop client?
Flags: needinfo?(rtestard)
uplift for desktop, there's minor string change to pick up in 37 - but not breaking.  stand-alone will be as soon as next update.
Whiteboard: [rooms] → [rooms][uplift - minor string pick up in 37]
This does the necessary changes to make the form always allow comments - Sevaan and RT both like the new functional changes (this version also fixes tests).
Attachment #8543964 - Attachment is obsolete: true
Attachment #8544647 - Flags: review?(nperriault)
The full patch this time, forgot the -a on amending the commit.
Attachment #8544647 - Attachment is obsolete: true
Attachment #8544647 - Flags: review?(nperriault)
Attachment #8544666 - Flags: review?(nperriault)
Comment on attachment 8544666 [details] [diff] [review]
Loop feedback form should always allow comments.

Review of attachment 8544666 [details] [diff] [review]:

Works great, thanks!
Attachment #8544666 - Flags: review?(nperriault) → review+

Will do a patch for aurora/beta later.
Target Milestone: --- → mozilla37
Patch for aurora without the string id changes - the colon change will be picked up by locales in the next versions, but lets do the best we can for now.
Attachment #8545415 - Flags: review?(nperriault)
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Comment on attachment 8545415 [details] [diff] [review]
Loop feedback form should always allow comments.

Review of attachment 8545415 [details] [diff] [review]:

LGTM. I've added comments from an IRC discussion to explain the string change.

::: browser/components/loop/standalone/content/l10n/en-US/
@@ +71,5 @@
>  feedback_category_audio_quality=Audio quality
>  feedback_category_video_quality=Video quality
>  feedback_category_was_disconnected=Was disconnected
>  feedback_category_confusing=Confusing
> +feedback_category_other=Other

[15:34:33]  Standard8:	so basically, I see it as although its changing the context of the string, its doesn’t actually matter too much, in that it’ll just lead to some possibly broken user assumptions for non-en-US locales.
[15:35:03]  Standard8:	but the broken assumptions will only be that it relates to the “other” category, and not the rest as well
[15:35:34]  Standard8:	We are allowed to change strings for punctuation errors (basically non-context) without changing the id
[15:39:07]  Standard8:
Attachment #8545415 - Flags: review?(nperriault) → review+
We decided to let this ride the Fx37 train (just updating the Fx36 blocking flag for clarity).
backlog: Fx36+ → Fx37+
You need to log in before you can comment on or make changes to this bug.