869255 - [MMS][User Interface] Attach button fire pick Activity and create Attachment

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[Dan Heberden]

There needs to be UI for the user to attach (pick) a media element. This should fire the pick activity, allowing the user to pick a media element, returning that element to the messaging app and adding to the message. (Something like `Compose.append(new Attachment(blob))`

Comment 1

[Dan Heberden]

This bug covers not only the pick button but also making sure that the styles (that the attach button will certainly affect) related to composition (the input box, send button, etc) are all up to spec.

Comment 2

[Mike Pennisi [:jugglinmike]]

I began implementing this interface component in Bug 840044. I've since marked that patch as obsolete, and I will attach a newer version here when it is available.

Comment 3

[Dan Heberden]

Revering this being so large of a bug. This is only for the pick icon and related activity. The test of the styling issues are https://bugzilla.mozilla.org/show_bug.cgi?id=870057

Comment 4

[Dan Heberden]

I mean the *bug* for the styling issues is https://bugzilla.mozilla.org/show_bug.cgi?id=870057

Comment 5

[Mike Pennisi [:jugglinmike]]

The "Search" building block[1] is documented to optionally support the "attachment" icon described by the UI specification. Unfortunately, it doesn't seem to technically implement this. I will reach out to the building block maintainers, but in the mean time, this patch will likely include an SMS-specific UI.

Comment 6

[Mike Pennisi [:jugglinmike]]

Attachment: https://github.com/bocoup/gaia/pull/13

Comment 7

[Mike Pennisi [:jugglinmike]]

Attachment: Implement Attach button UI

Corey has merged the "messaging" fork upstream, so I'm re-submitting this as a patch for review (and merge to "master") by Julien.

Comment 8

[Corey Frang [:gnarf]]

I kind of feel like this should be functionality & corresponding tests should baked into the Compose object, anyone else have opinions?

Comment 9

[Corey Frang [:gnarf]]

Also, this bug really feels co-dependant on bug 840044 - landing this one without the other is going to be confusing - a button that asks for a pick action, but doesn't actually do anything feels like a bug.  Perhaps we can agree on the commit for this one, and just land it at the same time as bug 840044?

Comment 10

[Mike Pennisi [:jugglinmike]]

Attachment: Implement Attach button UI

As per Corey's feedback, I've moved the relevant logic into `compose.js`

Comment 11

[Mike Pennisi [:jugglinmike]]

Attachment: Implement Attach button UI

As per This version moves more of the code that actually concerns message composition into 'compose.js'

Comment 12

[Mike Pennisi [:jugglinmike]]

Attachment: Implement Attach button UI

While working on Bug 840044, I realized that the test introduced in the previous patch excercised too many methods to be considered a unit test. This patch re-factors the test to be more focused.

Comment 13

[Corey Frang [:gnarf]]

Comment on attachment 749036 [details] [diff] [review]
Implement Attach button UI

Review of attachment 749036 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM - Just a few comments, but consider it r+ - let me know if you decide to make the follow up change

::: apps/sms/js/compose.js
@@ +114,5 @@
>      init: function thui_compose_init(formId) {
>        dom.form = document.getElementById(formId);
>        dom.message = dom.form.querySelector('[contenteditable]');
> +      dom.submitButton = dom.form.querySelector('button[type="submit"]');
> +      dom.attachButton = dom.form.querySelector('#messages-attach-button');

Should we use getElementById instead? Also maybe the same for the submit button?

@@ +270,5 @@
> +        }
> +      });
> +
> +      activity.onsuccess = function() {
> +         // TODO: use `activity.result` to create the attachment.

Not Blocking on this, but any TODO should have a bug # when possible

Comment 14

[Corey Frang [:gnarf]]

Also, the attach button seems incorrectly aligned, i would imagine it should be at the bottom like send is: http://cl.ly/image/3S0F2J190D3S

Comment 15

[Mike Pennisi [:jugglinmike]]

Attachment: Implement Attach button UI

Addressing Corey's feedback.

Comment 16

[Mike Pennisi [:jugglinmike]]

Attachment: Implement Attach button UI

This version addresses the rendering issue identified in comment 14

Comment 17

[Mike Pennisi [:jugglinmike]]

75e153f09e72b692917556e028dacb8436a0d86f

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 8725473