Skip to content

Commit

Permalink
[client-app] Fix measurement of thread-list action metrics
Browse files Browse the repository at this point in the history
Summary:
We detect when a thread is removed from the thread-list by listening to
`Actions.threadListDidUpdate`. However, we were not firing the action at
the correct moment.

Before this commit, we fired the action on the root `ThreadList`'s
`componentDidUpdate`, however did this not actually fire when the
child list actually updated/removed threads from the list.

This sort of used to work because before 396a027
the root ThreadList component re-rendered all the time, so it fired the
action, but after initial sync, we would never actually report any
thread-list action metrics at all

Test Plan: manual

Reviewers: spang, halla, evan

Reviewed By: halla, evan

Differential Revision: https://phab.nylas.com/D4051
  • Loading branch information
jstejada committed Feb 28, 2017
1 parent fa17d2f commit b6a1fc5
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,6 @@ class ThreadList extends React.Component
(not Utils.isEqualReact(@state, nextState))
)

componentDidUpdate: =>
dataSource = ThreadListStore.dataSource()
threads = dataSource.itemsCurrentlyInView()
Actions.threadListDidUpdate(threads)

componentWillUnmount: =>
@unsub()
window.removeEventListener('resize', @_onResize, true)
Expand Down Expand Up @@ -137,10 +132,16 @@ class ThreadList extends React.Component
onDoubleClick={(thread) -> Actions.popoutThread(thread)}
onDragStart={@_onDragStart}
onDragEnd={@_onDragEnd}
onComponentDidUpdate={@_onThreadListDidUpdate}
/>
</FocusContainer>
</FluxContainer>

_onThreadListDidUpdate: =>
dataSource = ThreadListStore.dataSource()
threads = dataSource.itemsCurrentlyInView()
Actions.threadListDidUpdate(threads)

_threadPropsProvider: (item) ->
classes = classnames({
'unread': item.unread
Expand Down
4 changes: 4 additions & 0 deletions packages/client-app/src/components/list-tabular.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ class ListTabular extends Component {
onDoubleClick: PropTypes.func,
onDragStart: PropTypes.func,
onDragEnd: PropTypes.func,
onComponentDidUpdate: PropTypes.func,
};

static defaultProps = {
Expand Down Expand Up @@ -133,6 +134,9 @@ class ListTabular extends Component {
}

componentDidUpdate(prevProps) {
if (this.props.onComponentDidUpdate) {
this.props.onComponentDidUpdate()
}
// If our view has been swapped out for an entirely different one,
// reset our scroll position to the top.
if (prevProps.dataSource !== this.props.dataSource) {
Expand Down
4 changes: 4 additions & 0 deletions packages/client-app/src/components/multiselect-list.cjsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class MultiselectList extends React.Component
columns: React.PropTypes.array.isRequired
itemPropsProvider: React.PropTypes.func.isRequired
keymapHandlers: React.PropTypes.object
onComponentDidUpdate: React.PropTypes.func

constructor: (@props) ->
@state = @_getStateFromStores()
Expand All @@ -48,6 +49,8 @@ class MultiselectList extends React.Component
@setState(@_getStateFromStores(newProps))

componentDidUpdate: (prevProps, prevState) =>
if @props.onComponentDidUpdate
@props.onComponentDidUpdate()
if prevProps.focusedId isnt @props.focusedId or
prevProps.keyboardCursorId isnt @props.keyboardCursorId

Expand Down Expand Up @@ -120,6 +123,7 @@ class MultiselectList extends React.Component
dataSource={@props.dataSource}
itemPropsProvider={@itemPropsProvider}
onSelect={@_onClickItem}
onComponentDidUpdate={@props.onComponentDidUpdate}
{...otherProps} />
</KeyCommandsRegion>
else
Expand Down

0 comments on commit b6a1fc5

Please sign in to comment.