fix(*): Small fixes for drafts, interface tweaks

Summary:
Message list can be narrower

Account sidebar is narrower

Never open new windows on single click

Blue send button

Clean up cruft from draft deletion

Render composer empty, setProps when draft populated

Use new `pristine` attribute to discard un-changed new drafts

_addToProxy needs deep equals to prevent "save to = [], cc = []"

Mark as read on click, not afterwards

Allow toolbar / sheet items to style based on the workspace mode

specs covering draft unloading / behavior of cleanup

Always, always reset mode to spec after each test

New tests for destroy draft functionality

Test Plan: Run a handful of new tests

Reviewers: evan

Reviewed By: evan

Differential Revision: https://review.inboxapp.com/D1335
This commit is contained in:
Ben Gotow 2015-03-23 16:33:28 -07:00
parent fe72c75dc5
commit 0669468ec0
19 changed files with 256 additions and 96 deletions

View file

@ -51,4 +51,4 @@ AccountSidebar = React.createClass
AccountSidebar.minWidth = 165
AccountSidebar.maxWidth = 250
AccountSidebar.maxWidth = 190

View file

@ -31,6 +31,9 @@ ComposerView = React.createClass
bcc: []
body: ""
subject: ""
showcc: false
showbcc: false
showsubject: false
showQuotedText: false
isSending: DraftStore.sendingState(@props.localId)
state
@ -41,8 +44,7 @@ ComposerView = React.createClass
FooterComponents: ComponentRegistry.findAllByRole 'Composer:Footer'
componentWillMount: ->
@_prepareForDraft()
# @_checkForKnownFrames()
@_prepareForDraft(@props.localId)
componentDidMount: ->
@_draftStoreUnlisten = DraftStore.listen @_onSendingStateChanged
@ -64,9 +66,6 @@ ComposerView = React.createClass
@_draftStoreUnlisten() if @_draftStoreUnlisten
@keymap_unsubscriber.dispose()
componentWillUpdate: ->
#@_checkForKnownFrames()
componentDidUpdate: ->
# We want to use a temporary variable instead of putting this into the
# state. This is because the selection is a transient property that
@ -76,28 +75,31 @@ ComposerView = React.createClass
@_recoveredSelection = null if @_recoveredSelection?
componentWillReceiveProps: (newProps) ->
if newProps.localId != @props.localId
if newProps.localId isnt @props.localId
# When we're given a new draft localId, we have to stop listening to our
# current DraftStoreProxy, create a new one and listen to that. The simplest
# way to do this is to just re-call registerListeners.
@_teardownForDraft()
@_prepareForDraft()
_prepareForDraft: ->
# UndoManager must be ready before we call _onDraftChanged for the first time
@undoManager = new UndoManager
@_proxy = DraftStore.sessionForLocalId(@props.localId)
if @_proxy.draft()
@_onDraftChanged()
@_prepareForDraft(newProps.localId)
_prepareForDraft: (localId) ->
@unlisteners = []
@unlisteners.push @_proxy.listen(@_onDraftChanged)
@unlisteners.push ComponentRegistry.listen (event) =>
@setState(@getComponentRegistryState())
return unless localId
# UndoManager must be ready before we call _onDraftChanged for the first time
@undoManager = new UndoManager
@_proxy = DraftStore.sessionForLocalId(localId)
@unlisteners.push @_proxy.listen(@_onDraftChanged)
if @_proxy.draft()
@_onDraftChanged()
_teardownForDraft: ->
unlisten() for unlisten in @unlisteners
@_proxy.changes.commit()
if @_proxy
@_proxy.changes.commit()
render: ->
if @props.mode is "inline"
@ -210,10 +212,10 @@ ComposerView = React.createClass
data-tooltip="Attach file"
onClick={@_attachFile}><RetinaImg name="toolbar-attach.png"/></button>
<button className="btn btn-toolbar btn-send"
<button className="btn btn-toolbar btn-emphasis btn-send"
data-tooltip="Send message"
ref="sendButton"
onClick={@_sendDraft}><RetinaImg name="toolbar-send.png" /></button>
onClick={@_sendDraft}><RetinaImg name="toolbar-send.png" /> Send</button>
{@_actionButtonComponents()}
</div>
</div>
@ -241,10 +243,12 @@ ComposerView = React.createClass
Utils.isForwardedMessage(draft.body, draft.subject)
_actionButtonComponents: ->
return [] unless @props.localId
(@state.ActionButtonComponents ? []).map ({view, name}) =>
<view key={name} draftLocalId={@props.localId} />
_footerComponents: ->
return [] unless @props.localId
(@state.FooterComponents ? []).map ({view, name}) =>
<view key={name} draftLocalId={@props.localId} />
@ -302,10 +306,12 @@ ComposerView = React.createClass
@setState showQuotedText: showQuotedText
_addToProxy: (changes={}, source={}) ->
return unless @_proxy
selections = @_getSelections()
oldDraft = @_proxy.draft()
return if _.all changes, (change, key) -> change == oldDraft[key]
return if _.all changes, (change, key) -> _.isEqual(change, oldDraft[key])
@_proxy.changes.add(changes)
@_saveToHistory(selections) unless source.fromUndoManager
@ -377,23 +383,10 @@ ComposerView = React.createClass
@setState {showcc: true}
@focus "textFieldCc"
# Warning this method makes optimistic assumptions about the mail client
# and is not properly encapsulated.
_checkForKnownFrames: ->
@_precalcComposerCss = {}
mwrap = document.getElementsByClassName("messages-wrap")[0]
if mwrap?
INLINE_COMPOSER_OTHER_HEIGHT = 192
mheight = mwrap.getBoundingClientRect().height
@_precalcComposerCss =
minHeight: mheight - INLINE_COMPOSER_OTHER_HEIGHT
_onSendingStateChanged: ->
@setState isSending: DraftStore.sendingState(@props.localId)
undo: (event) ->
event.preventDefault()
event.stopPropagation()

View file

@ -29,16 +29,16 @@ module.exports =
@item.setAttribute("class", "composer-full-window")
document.body.appendChild(@item)
component = React.render(<ComposerView mode="fullwindow" />, @item)
# Wait for the remaining state to be passed into the window
# from our parent. We need to wait for state because the windows are
# preloaded so they open instantly, so we don't have data initially
ipc.on 'composer-state', (optionsJSON) =>
options = JSON.parse(optionsJSON)
@_createDraft(options).then (draftLocalId) =>
React.render(<ComposerView mode="fullwindow" localId={draftLocalId} />, @item)
_.delay =>
if options.error? then @_showInitialErrorDialog(options.error)
, 100
component.setProps {localId: draftLocalId}, =>
@_showInitialErrorDialog(options.error) if options.error?
.catch (error) -> console.error(error)
@ -71,6 +71,7 @@ module.exports =
from: [NamespaceStore.current().me()]
date: (new Date)
draft: true
pristine: true
namespaceId: NamespaceStore.current().id
# If initial JSON was provided, apply it to the new model.
# This is used to apply the values in mailto: links to new drafts

View file

@ -173,5 +173,5 @@ MessageList = React.createClass
participants[contact.email] = contact
return _.values(participants)
MessageList.minWidth = 680
MessageList.minWidth = 500
MessageList.maxWidth = 900

View file

@ -44,6 +44,11 @@
}
}
.mode-split {
.message-toolbar-subject {
margin-left:@padding-base-horizontal;
}
}
#message-list {
display: flex;
flex-direction: row;

View file

@ -37,7 +37,7 @@ DraftList = React.createClass
columns={@state.columns}
items={@state.items}
selectedId={@state.selectedId}
onClick={@_onClick}
onDoubleClick={@_onDoubleClick}
onSelect={@_onSelect} />
</div>
@ -45,7 +45,7 @@ DraftList = React.createClass
@setState
selectedId: item.id
_onClick: (item) ->
_onDoubleClick: (item) ->
DatabaseStore.localIdForModel(item).then (localId) ->
Actions.composePopoutDraft(localId)

View file

@ -5,6 +5,7 @@ NamespaceStore = require '../../src/flux/stores/namespace-store.coffee'
DatabaseStore = require '../../src/flux/stores/database-store.coffee'
DraftStore = require '../../src/flux/stores/draft-store.coffee'
SendDraftTask = require '../../src/flux/tasks/send-draft'
DestroyDraftTask = require '../../src/flux/tasks/destroy-draft'
Actions = require '../../src/flux/actions'
_ = require 'underscore-plus'
@ -266,6 +267,84 @@ describe "DraftStore", ->
, (thread, message) ->
expect(message).toEqual(fakeMessage1)
{}
describe "onDestroyDraft", ->
beforeEach ->
@draftReset = jasmine.createSpy('draft reset')
spyOn(Actions, 'queueTask')
DraftStore._draftSessions = {"abc":{
draft: ->
pristine: false
changes:
commit: -> Promise.resolve()
reset: @draftReset
cleanup: ->
}}
it "should reset the draft session, ensuring no more saves are made", ->
DraftStore._onDestroyDraft('abc')
expect(@draftReset).toHaveBeenCalled()
it "should not do anything if the draft session is not in the window", ->
expect ->
DraftStore._onDestroyDraft('other')
.not.toThrow()
expect(@draftReset).not.toHaveBeenCalled()
it "should queue a destroy draft task", ->
DraftStore._onDestroyDraft('abc')
expect(Actions.queueTask).toHaveBeenCalled()
expect(Actions.queueTask.mostRecentCall.args[0] instanceof DestroyDraftTask).toBe(true)
it "should clean up the draft session", ->
spyOn(DraftStore, 'cleanupSessionForLocalId')
DraftStore._onDestroyDraft('abc')
expect(DraftStore.cleanupSessionForLocalId).toHaveBeenCalledWith('abc')
describe "before unloading", ->
it "should destroy pristine drafts", ->
DraftStore._draftSessions = {"abc": {
changes: {}
draft: ->
pristine: true
}}
spyOn(Actions, 'queueTask')
DraftStore._onBeforeUnload()
expect(Actions.queueTask).toHaveBeenCalled()
expect(Actions.queueTask.mostRecentCall.args[0] instanceof DestroyDraftTask).toBe(true)
describe "when drafts return unresolved commit promises", ->
beforeEach ->
@resolve = null
DraftStore._draftSessions = {"abc": {
changes:
commit: => new Promise (resolve, reject) => @resolve = resolve
draft: ->
pristine: false
}}
it "should return false and call window.close itself", ->
spyOn(window, 'close')
expect(DraftStore._onBeforeUnload()).toBe(false)
runs ->
@resolve()
waitsFor ->
window.close.callCount > 0
runs ->
expect(window.close).toHaveBeenCalled()
describe "when no drafts return unresolved commit promises", ->
beforeEach ->
DraftStore._draftSessions = {"abc":{
changes:
commit: -> Promise.resolve()
draft: ->
pristine: false
}}
it "should return true and allow the window to close", ->
expect(DraftStore._onBeforeUnload()).toBe(true)
describe "sending a draft", ->
draftLocalId = "local-123"
@ -274,13 +353,12 @@ describe "DraftStore", ->
DraftStore._draftSessions = {}
DraftStore._draftSessions[draftLocalId] =
prepare: -> Promise.resolve()
cleanup: ->
draft: -> {}
changes:
commit: -> Promise.resolve()
spyOn(DraftStore, "trigger")
afterEach ->
atom.state.mode = "editor" # reset to default
it "sets the sending state when sending", ->
DraftStore._onSendDraft(draftLocalId)
expect(DraftStore.sendingState(draftLocalId)).toBe true
@ -336,3 +414,57 @@ describe "DraftStore", ->
expect(Actions.queueTask).toHaveBeenCalled()
task = Actions.queueTask.calls[0].args[0]
expect(task.fromPopout).toBe true
describe "cleanupSessionForLocalId", ->
it "should destroy the draft if it is pristine", ->
DraftStore._draftSessions = {"abc":{
draft: ->
pristine: true
cleanup: ->
}}
spyOn(Actions, 'queueTask')
DraftStore.cleanupSessionForLocalId('abc')
expect(Actions.queueTask).toHaveBeenCalled()
expect(Actions.queueTask.mostRecentCall.args[0] instanceof DestroyDraftTask).toBe(true)
it "should not do anything bad if the session does not exist", ->
expect ->
DraftStore.cleanupSessionForLocalId('dne')
.not.toThrow()
describe "when in the popout composer", ->
beforeEach ->
atom.state.mode = 'composer'
DraftStore._draftSessions = {"abc":{
draft: ->
pristine: false
cleanup: ->
}}
it "should close the composer window", ->
spyOn(atom, 'close')
DraftStore.cleanupSessionForLocalId('abc')
expect(atom.close).toHaveBeenCalled()
it "should not close the composer window if the draft session is not in the window", ->
spyOn(atom, 'close')
DraftStore.cleanupSessionForLocalId('other-random-draft-id')
expect(atom.close).not.toHaveBeenCalled()
describe "when it is in a main window", ->
beforeEach ->
@cleanup = jasmine.createSpy('cleanup')
DraftStore._draftSessions = {"abc":{
draft: ->
pristine: false
cleanup: @cleanup
}}
it "should call proxy.cleanup() to unlink listeners", ->
DraftStore.cleanupSessionForLocalId('abc')
expect(@cleanup).toHaveBeenCalled()
it "should remove the proxy from the sessions list", ->
DraftStore.cleanupSessionForLocalId('abc')
expect(DraftStore._draftSessions).toEqual({})

View file

@ -160,7 +160,8 @@ afterEach ->
atom.packages.deactivatePackages()
atom.menu.template = []
atom.contextMenu.clear()
atom.state.mode = 'spec'
atom.themes.removeStylesheet('global-editor-styles')
delete atom.state.packageStates

View file

@ -22,9 +22,7 @@ globalActions = [
# Draft actions
"sendDraftError",
"sendDraftSuccess",
"destroyDraftSuccess",
"destroyDraftError"
"sendDraftSuccess"
]
# These actions are rebroadcast through the ActionBridge to the

View file

@ -56,6 +56,11 @@ class Message extends Model
jsonKey: 'draft'
queryable: true
'pristine': Attributes.Boolean
modelKey: 'pristine'
jsonKey: 'pristine'
queryable: false
'version': Attributes.Number
modelKey: 'version'
queryable: true
@ -111,5 +116,4 @@ class Message extends Model
fileIds: ->
_.map @files, (file) -> file.id
module.exports = Message

View file

@ -31,8 +31,6 @@ AnalyticsStore = Reflux.createStore
fileUploaded: (uploadData={}) -> {fileSize: uploadData.fileSize}
sendDraftError: (dId, msg) -> {drafLocalId: dId, error: msg}
sendDraftSuccess: (draftLocalId) -> {draftLocalId: draftLocalId}
destroyDraftSuccess: -> {}
destroyDraftError: (msg) -> {error: msg}
showDeveloperConsole: -> {}
composeReply: ({threadId, messageId}) -> {threadId, messageId}
composeForward: ({threadId, messageId}) -> {threadId, messageId}

View file

@ -25,6 +25,7 @@ class DraftChangeSet
add: (changes, immediate) =>
@_pending = _.extend(@_pending, changes)
@_pending['pristine'] = false
@_onChange()
if immediate
@commit()
@ -39,8 +40,8 @@ class DraftChangeSet
DatabaseStore = require './database-store'
DatabaseStore.findByLocalId(Message, @localId).then (draft) =>
draft = @applyToModel(draft)
@_pending = {}
DatabaseStore.persistModel(draft)
DatabaseStore.persistModel(draft).then =>
@_pending = {}
applyToModel: (model) =>
model.fromJSON(@_pending) if model
@ -93,6 +94,9 @@ class DraftStoreProxy
@_emitter.addListener('trigger', eventHandler)
return =>
@_emitter.removeListener('trigger', eventHandler)
if @_emitter.listeners('trigger').length is 0
DraftStore = require './draft-store'
DraftStore.cleanupSessionForLocalId(@draftLocalId)
cleanup: ->
# Unlink ourselves from the stores/actions we were listening to

View file

@ -44,7 +44,6 @@ DraftStore = Reflux.createStore
@listenTo Actions.sendDraftError, @_onSendDraftSuccess
@listenTo Actions.sendDraftSuccess, @_onSendDraftError
@listenTo Actions.destroyDraftSuccess, @_closeWindow
@_drafts = []
@_draftSessions = {}
@_sendingState = {}
@ -52,28 +51,7 @@ DraftStore = Reflux.createStore
# TODO: Doesn't work if we do window.addEventListener, but this is
# fragile. Pending an Atom fix perhaps?
window.onbeforeunload = (event) =>
promises = []
# Normally we'd just append all promises, even the ones already
# fulfilled (nothing to save), but in this case we only want to
# block window closing if we have to do real work. Calling
# window.close() within on onbeforeunload could do weird things.
for key, session of @_draftSessions
promise = session.changes.commit()
if not promise.isFulfilled()
promises.push(promise)
if promises.length > 0
Promise.settle(promises).then =>
@_draftSessions = {}
window.close()
# Stop and wait before closing
return false
else
# Continue closing
return true
window.onbeforeunload = => @_onBeforeUnload()
DatabaseStore.findAll(Message, draft: true).then (drafts) =>
@_drafts = drafts
@ -87,6 +65,7 @@ DraftStore = Reflux.createStore
@_drafts
sessionForLocalId: (localId) ->
throw new Error("sessionForLocalId requires a localId") unless localId
@_draftSessions[localId] ?= new DraftStoreProxy(localId)
@_draftSessions[localId]
@ -104,6 +83,43 @@ DraftStore = Reflux.createStore
@_extensions = _.without(@_extensions, ext)
########### PRIVATE ####################################################
cleanupSessionForLocalId: (localId) ->
return unless @_draftSessions[localId]
draft = @_draftSessions[localId].draft()
Actions.queueTask(new DestroyDraftTask(localId)) if draft.pristine
if atom.state.mode is "composer"
atom.close()
else
@_draftSessions[localId].cleanup()
delete @_draftSessions[localId]
_onBeforeUnload: ->
promises = []
# Normally we'd just append all promises, even the ones already
# fulfilled (nothing to save), but in this case we only want to
# block window closing if we have to do real work. Calling
# window.close() within on onbeforeunload could do weird things.
for key, session of @_draftSessions
if session.draft()?.pristine
Actions.queueTask(new DestroyDraftTask(session.draftLocalId))
else
promise = session.changes.commit()
promises.push(promise) unless promise.isFulfilled()
if promises.length > 0
Promise.settle(promises).then =>
@_draftSessions = {}
window.close()
# Stop and wait before closing
return false
else
# Continue closing
return true
_onDataChanged: (change) ->
return unless change.objectClass is Message.name
@ -199,17 +215,12 @@ DraftStore = Reflux.createStore
from: [NamespaceStore.current().me()]
date: (new Date)
draft: true
pristine: true
threadId: thread.id
namespaceId: thread.namespaceId
DatabaseStore.persistModel(draft)
# We only want to close the popout window if we're sure various draft
# actions succeeded.
_closeWindow: (draftLocalId) ->
if atom.state.mode is "composer" and @_draftSessions[draftLocalId]?
atom.close()
# The logic to create a new Draft used to be in the DraftStore (which is
# where it should be). It got moved to composer/lib/main.cjsx becaues
# of an obscure atom-shell/Chrome bug whereby database requests firing right
@ -224,13 +235,14 @@ DraftStore = Reflux.createStore
_onDestroyDraft: (draftLocalId) ->
# Immediately reset any pending changes so no saves occur
@_closeWindow(draftLocalId)
@_draftSessions[draftLocalId]?.changes.reset()
delete @_draftSessions[draftLocalId]
# Queue the task to destroy the draft
Actions.queueTask(new DestroyDraftTask(draftLocalId))
# Clean up the draft session
@cleanupSessionForLocalId(draftLocalId)
_onSendDraft: (draftLocalId) ->
new Promise (resolve, reject) =>
@_sendingState[draftLocalId] = true
@ -245,12 +257,13 @@ DraftStore = Reflux.createStore
# Immediately save any pending changes so we don't save after sending
session.changes.commit().then =>
# We optimistically close the window. If we get an error, then it
# will re-open again.
@_closeWindow(draftLocalId)
# Queue the task to send the draft
fromPopout = atom.state.mode is "composer"
Actions.queueTask(new SendDraftTask(draftLocalId, fromPopout: fromPopout))
# Clean up session, close window
@cleanupSessionForLocalId(draftLocalId)
resolve()
_onSendDraftError: (draftLocalId) ->

View file

@ -103,14 +103,13 @@ ThreadStore = Reflux.createStore
@fetchFromAPI()
_onSelectThreadId: (id) ->
# Mark the *previously* selected thread as read,
# before we bring in the next thread
return if @_selectedId == id
@_selectedId = id
thread = @selectedThread()
if thread && thread.isUnread()
thread.markAsRead()
return if @_selectedId == id
@_selectedId = id
@trigger()
# Accessing Data

View file

@ -91,7 +91,7 @@ Toolbar = React.createClass
{@_flexboxForItems(items)}
</div>
<div style={style}>
<div style={style} className={"mode-#{@state.mode}"}>
{toolbars}
</div>

View file

@ -51,6 +51,7 @@ Sheet = React.createClass
<div name={"Sheet"}
style={style}
className={"mode-#{@state.mode}"}
data-type={@props.data.type}>
<Flexbox direction="row">
{@_columnFlexboxElements()}
@ -84,7 +85,7 @@ Sheet = React.createClass
</Flexbox>
_getStateFromStores: ->
state =
state =
mode: WorkspaceStore.selectedLayoutMode()
columns: []

View file

@ -46,14 +46,26 @@ button, html input[type="button"] {
color: @btn-default-text-color;
background: @btn-default-bg-color;
&.btn-action {
color: @btn-action-text-color;
background: @btn-action-bg-color;
}
&.btn-emphasis {
color: @btn-emphasis-text-color;
background: @btn-emphasis-bg-color;
background-image: -webkit-gradient(linear, left top, left bottom, from(lighten(@btn-emphasis-bg-color,10%)), to(@btn-emphasis-bg-color));
border:1px solid darken(@btn-emphasis-bg-color, 5%);
color: @btn-emphasis-text-color;
font-weight: @font-weight-medium;
img {-webkit-filter: brightness(100);}
}
&.btn-emphasis:active {
background-image: -webkit-gradient(linear, left top, left bottom, from(darken(@btn-emphasis-bg-color,10%)), to(darken(@btn-emphasis-bg-color, 4%)));
box-shadow: 0 1px 1px rgba(0, 0, 0, 0.21);
}
&.btn-danger, .btn-destructive {
color: @btn-danger-text-color;
background: @btn-danger-bg-color;

Binary file not shown.

Before

Width:  |  Height:  |  Size: 53 KiB

After

Width:  |  Height:  |  Size: 51 KiB

View file

@ -236,7 +236,7 @@
@btn-action-bg-color: @success-color;
@btn-action-text-color: @text-color;
@btn-emphasis-bg-color: @accent-primary;
@btn-emphasis-bg-color: #5b90fb;
@btn-emphasis-text-color: @text-color-inverse;
@btn-danger-bg-color: @danger-color;
@ -429,7 +429,6 @@
@teal: @PANTONE-326-UP;
@black: @PANTONE-Process-Black-UP;
@cool-gray: @PANTONE-Cool-Gray-1-UP;
@white: #f1f1f1;
@blue-grey: @blue-gray;
@light-grey: @light-gray;