Bug 936647 - [Messages][Drafts] Preview drafts threads in thread list #1

rwaldron · 2013-11-30T02:36:34Z

https://bugzilla.mozilla.org/show_bug.cgi?id=936647

index.html:
- messages-thread-tmpl
  - Adds "icon-draft" class to "icon icon-read" classes
  - Adds data-l10n-id="no-receipient"
  - Adds data-list=draft|thread
js/desktop-only/mobilemessage.js
- Updates Draft bootstrapping data
js/drafts.js
- Adds read-once and cache to Drafts.load
- Adds unique id mechanism
- Updates Drafts.delete to allow deleting all drafts for a threadId
- Renames Drafts.byId => Drafts.byThreadId
- Adds Drafts.get(draftId)
- Updates Drafts.load to abide a local isCached flag.
  - Only load from storage when isCached is false.
- Updates Drafts.load to revive stored Draft objects as actual Draft objects
- Adds Draft.List.prototype.latest that points at the latest draft in a Draft.List (ie. for a specified threadId or null key)
- Adds function guid for creating quasi-unique ids
js/message_manager.js
- Adds draft property to hold draft object to coordinate with New Message view
- Calls Drafts.load() from init
- Calls ThreadListUI.renderDrafts() from onHashChange when hash = #thread-list
js/thread_list_ui.js
- Wraps contents in IIFE
- Adds draftLinks property to map DOM nodes to draftId
- Updates setContact to include lookup from draft
- Updates handleEvent to capture draft object on click. Stops looking at 'threads-container'
- Updates delete to create two lists of deletable items from the selectedInputs array. List is determined by data-list property value.
  - Drafts are deleted similarly to Threads, but will exit immediately if no Threads are to be deleted
  - Drafts.store() is called upon completing delete operation
- Adds ThreadListUI.renderDrafts to create "Thread List Items" from Draft objects that do not have their own thread
- Calls ThreadListUI.renderDrafts from ThreadListUI.renderThreads
- Updates ThreadListUI.createThread to create a "Thread List Item" from a Draft object, or to mark a Thread that has a draft.
- Updates rendered markup for "Thread List Item"
- Updates timestamp handling in ThreadListUI.appendThread
js/thread_ui.js
- Updates ThreadUI.saveMessageDraft new or replacement drafts
js/threads.js
- Expose Thread constructor
- Adds Thread.prototype.drafts accessor to Drafts.byThreadId(this.id);
- Adds Thread.prototype.hasDrafts accessor as short hand
Adds tests to:
- test/unit/compose_test.js
- test/unit/drafts_test.js
- test/unit/media/pixel.jpg
- test/unit/message_manager_test.js
- test/unit/mock_async_storage.js
- test/unit/mock_drafts.js
- test/unit/mock_navigatormoz_sms.js
- test/unit/thread_list_ui_test.js
- test/unit/thread_ui_test.js
- test/unit/threads_test.js
Passing both lint and hint

evhan55 · 2013-12-02T15:10:39Z

apps/sms/js/drafts.js

+
+        threadId = draft.threadId || null;
+        thread = draftIndex.get(threadId) || [];
+        stored = thread[thread.length - 1];

        // If the new draft is distinct from the stored one
        // then push and save a new draft
        if (isDistinct(stored, draft)) {
          thread.push(draft);


So, I had a question. Did we decide to only ever store one draft per threadId?
If so, shouldn't Drafts.add remove the old draft at that threadId if it exists?
This has never been written into the add, and I wonder if we want to change it to do that.

Is there any case where we want to store old drafts for a single thread?

The "no recipient" thread maybe?

@evhan55 This makes sense.

julienw · 2013-12-03T16:21:59Z

lots of jshint errors

julienw · 2013-12-03T16:23:52Z

gjslint issues as well btw ;)

julienw · 2013-12-03T16:25:05Z

apps/sms/index.html

        <aside class="pack-end">
          <img src="">
        </aside>
-        <p class="name">${number}</p>
+        <p class="name" data-l10n-id="no-receipient">${number}</p>


nit: recipient

rwaldron · 2013-12-03T16:31:09Z

@julienw both the lint and hint errors were fixed probably at the same moment you wrote those :)

julienw · 2013-12-03T17:13:24Z

apps/sms/style/sms.css

+}
+
+[data-type="list"] .draft aside.icon-draft {
+  background: url("../style/images/draft.png") no-repeat left center / 1rem;


3 comments on this:

can we factorize with icon-unread? Have an "icon" class for example? Only the background-image changes between states.

can you investigate if we can have a .draft class on the thread node itself? And .unread too? Maybe even make the icon a generated content (although I'm not sure of this one, maybe it's overkill) ?

I think we can have both unread and draft, and your current markup does not allow this.

can we factorize with icon-unread? Have an "icon" class for example? Only the background-image changes between states.

We could, but there is already a class called .icon. Maybe a more thorough cleanup should be done, but in a different patch?

can you investigate if we can have a .draft class on the thread node itself? And .unread too? Maybe even make the icon a generated content (although I'm not sure of this one, maybe it's overkill) ?

We do, for setting the opacity to 0.5

I think we can have both unread and draft, and your current markup does not allow this.

I'm not sure how that's possible (visually)—the two icons exist at the exact same place :(

ok, I get it, we can have basically these states:

a "pure draft", that is a "new message draft", has is semi transparent and has the pencil icon

a "thread draft" has the pencil icon but is fully opaque

Now, what should we have for threads that have a draft but there is an unread message in it? IINM This means the most recent message in the thread is more recent than when the draft was created (because when the draft is created, all messages are read)

This is both a UX+Visual Design issue, let me raise this on the bug

that said, I'm open with moving forward and fixing whatever nees to be fixed here in a follow-up

that said, I'm open with moving forward and fixing whatever nees to be fixed here in a follow-up

Ok, good to know and I agree

julienw · 2013-12-03T18:32:54Z

apps/sms/js/thread_ui.js

+      // the drafts timestamp.
+      thread.timestamp = draft.timestamp;
+
+      ThreadListUI.appendThread(thread);


can you look if ThreadListUI.updateThread is usable?

Fixed.

This required implementing an updated Thread.create (ie. rewriting Threads.createThreadMockup since ThreadListUI.updateThread was expected to call through to that function for creating thread objects from messages

… prompt to options menu/dialog; add "save draft" no-op r=julienw https://bugzilla.mozilla.org/show_bug.cgi?id=935177 - New strings: - save-as-draft = Save as Draft - discard-message = Discard - Removes window.confirm mechanism - Implements Multi-option prompt with OptionMenu - Basic tests to assert the correct operations are performed by the option handlers Signed-off-by: Rick Waldron <waldron.rick@gmail.com>

…action

… r=julienw https://bugzilla.mozilla.org/show_bug.cgi?id=936647 - index.html: - messages-thread-tmpl - Adds "icon-draft" class to "icon icon-read" classes - Adds data-l10n-id="no-receipient" - Adds data-list=draft|thread - js/desktop-only/mobilemessage.js - Updates Draft bootstrapping data - js/drafts.js - Adds read-once and cache to Drafts.request - Adds unique id mechanism - Updates Drafts.delete to allow deleting all drafts for a threadId - Renames Drafts.byId => Drafts.byThreadId - Adds Drafts.get(draftId) - Updates Drafts.request to abide a local isCached flag. - Only load from storage when isCached is false. - Updates Drafts.request to revive stored Draft objects as actual Draft objects - Adds Draft.List.prototype.latest that points at the latest draft in a Draft.List (ie. for a specified threadId or null key) - Adds function guid for creating quasi-unique ids - js/message_manager.js - Adds `draft` property to hold draft object to coordinate with New Message view - Calls Drafts.request() from init - Calls ThreadListUI.renderDrafts() from onHashChange when hash = #thread-list - js/thread_list_ui.js - Wraps contents in IIFE - Adds draftLinks property to map DOM nodes to draftId - Updates setContact to include lookup from draft - Updates handleEvent to capture draft object on click. Stops looking at 'threads-container' - Updates delete to create two lists of deletable items from the selectedInputs array. List is determined by data-list property value. - Drafts are deleted similarly to Threads, but will exit immediately if no Threads are to be deleted - Drafts.store() is called upon completing delete operation - Adds ThreadListUI.renderDrafts to create "Thread List Items" from Draft objects that do not have their own thread - Calls ThreadListUI.renderDrafts from ThreadListUI.renderThreads - Updates ThreadListUI.createThread to create a "Thread List Item" from a Draft object, or to mark a Thread that has a draft. - Updates rendered markup for "Thread List Item" - Updates timestamp handling in ThreadListUI.appendThread - js/thread_ui.js - Updates ThreadUI.saveMessageDraft new or replacement drafts - js/threads.js - Expose Thread constructor - Adds Thread.prototype.drafts accessor to Drafts.byThreadId(this.id); - Adds Thread.prototype.hasDrafts accessor as short hand - Adds tests to: - test/unit/compose_test.js - test/unit/drafts_test.js - test/unit/media/pixel.jpg - test/unit/message_manager_test.js - test/unit/mock_async_storage.js - test/unit/mock_drafts.js - test/unit/mock_navigatormoz_sms.js - test/unit/thread_list_ui_test.js - test/unit/thread_ui_test.js - test/unit/threads_test.js - Passing both lint and hint Signed-off-by: Rick Waldron <waldron.rick@gmail.com>

add runtime customize for bookmark setting, r=yurenju

evhan55 reviewed Dec 2, 2013
View reviewed changes

rwaldron closed this Dec 3, 2013

rwaldron deleted the 936647 branch December 3, 2013 15:32

rwaldron reopened this Dec 3, 2013

rwaldron restored the 936647 branch December 3, 2013 15:35

julienw reviewed Dec 3, 2013
View reviewed changes

rwaldron and others added 3 commits December 4, 2013 17:51

Bug 936641 - [Messages][Drafts] Create message drafts upon user inter…

69780a6

…action

rwaldron pushed a commit that referenced this pull request Feb 18, 2014

Merge pull request #1 from gasolin/patch-1

baf19d7

add runtime customize for bookmark setting, r=yurenju

evhan55 closed this Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 936647 - [Messages][Drafts] Preview drafts threads in thread list #1

Bug 936647 - [Messages][Drafts] Preview drafts threads in thread list #1

rwaldron commented Nov 30, 2013

evhan55 Dec 2, 2013

gnarf Dec 2, 2013

rwaldron Dec 2, 2013

julienw commented Dec 3, 2013

julienw commented Dec 3, 2013

julienw Dec 3, 2013

rwaldron Dec 3, 2013

rwaldron commented Dec 3, 2013

julienw Dec 3, 2013

rwaldron Dec 3, 2013

julienw Dec 4, 2013

julienw Dec 4, 2013

rwaldron Dec 4, 2013

julienw Dec 3, 2013

rwaldron Dec 3, 2013

Bug 936647 - [Messages][Drafts] Preview drafts threads in thread list #1

Bug 936647 - [Messages][Drafts] Preview drafts threads in thread list #1

Conversation

rwaldron commented Nov 30, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw commented Dec 3, 2013

julienw commented Dec 3, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaldron commented Dec 3, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment