From 2c25b6c0c16437e32f4e80bc330708c9be1a5226 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Tue, 18 Jun 2019 00:11:08 -0300 Subject: [PATCH] MM-16152: always focus RHS on SELECT_POST Ideally, we'd refactor focus to be an entity we can directly manipulate in a reducer, but that's a non-trivial refactoring. For now, I'm capturing the timestamp of the last `SELECT_POST` and using that within the component to decide when to ignore/effect a focus of the RHS textbox, given that we already track the blur timestamp. --- actions/post_actions.jsx | 1 + actions/views/rhs.js | 9 ++- actions/views/rhs.test.js | 40 ++++++---- components/create_comment/create_comment.jsx | 7 +- .../create_comment/create_comment.test.jsx | 78 ++++++++++++++++--- components/create_comment/index.js | 3 +- reducers/views/rhs.js | 12 +++ reducers/views/rhs.test.js | 7 ++ selectors/rhs.jsx | 4 + selectors/rhs.test.js | 12 +++ 10 files changed, 143 insertions(+), 30 deletions(-) diff --git a/actions/post_actions.jsx b/actions/post_actions.jsx index a618fd84b1cc..45a22e7f4adf 100644 --- a/actions/post_actions.jsx +++ b/actions/post_actions.jsx @@ -243,6 +243,7 @@ export function deleteAndRemovePost(post) { type: ActionTypes.SELECT_POST, postId: '', channelId: '', + timestamp: 0, }); } diff --git a/actions/views/rhs.js b/actions/views/rhs.js index e9b5960f6753..2a8d8ec62c20 100644 --- a/actions/views/rhs.js +++ b/actions/views/rhs.js @@ -50,6 +50,7 @@ export function selectPostFromRightHandSideSearch(post) { postId: postRootId, channelId: post.channel_id, previousRhsState: getRhsState(getState()), + timestamp: Date.now(), }); }; } @@ -209,6 +210,7 @@ export function closeRightHandSide() { type: ActionTypes.SELECT_POST, postId: '', channelId: '', + timestamp: 0, }, ])); }; @@ -240,7 +242,12 @@ export function toggleRhsExpanded() { } export function selectPost(post) { - return {type: ActionTypes.SELECT_POST, postId: post.root_id || post.id, channelId: post.channel_id}; + return { + type: ActionTypes.SELECT_POST, + postId: post.root_id || post.id, + channelId: post.channel_id, + timestamp: Date.now(), + }; } export function selectPostCard(post) { diff --git a/actions/views/rhs.test.js b/actions/views/rhs.test.js index ebf96f940e3e..c2bd184702d1 100644 --- a/actions/views/rhs.test.js +++ b/actions/views/rhs.test.js @@ -35,6 +35,10 @@ const currentUserId = 'user123'; const UserSelectors = require('mattermost-redux/selectors/entities/users'); UserSelectors.getCurrentUserMentionKeys = jest.fn(() => [{key: '@here'}, {key: '@mattermost'}, {key: '@channel'}, {key: '@all'}]); +// Mock Date.now() to return a constant value. +const POST_CREATED_TIME = Date.now(); +global.Date.now = jest.fn(() => POST_CREATED_TIME); + jest.mock('mattermost-redux/actions/posts', () => ({ getPostThread: (...args) => ({type: 'MOCK_GET_POST_THREAD', args}), getProfilesAndStatusesForPosts: (...args) => ({type: 'MOCK_GET_PROFILES_AND_STATUSES_FOR_POSTS', args}), @@ -120,26 +124,29 @@ describe('rhs view actions', () => { expect(store.getActions()[0]).toEqual(compareStore.getActions()[0]); }); - test(`it dispatches ${ActionTypes.SELECT_POST} correctly`, async () => { - store = mockStore({ - ...initialState, - views: { - rhs: { - rhsState: RHSStates.FLAG, + describe(`it dispatches ${ActionTypes.SELECT_POST} correctly`, () => { + it('with mocked date', async () => { + store = mockStore({ + ...initialState, + views: { + rhs: { + rhsState: RHSStates.FLAG, + }, }, - }, - }); + }); - await store.dispatch(selectPostFromRightHandSideSearch(post)); + await store.dispatch(selectPostFromRightHandSideSearch(post)); - const action = { - type: ActionTypes.SELECT_POST, - postId: post.root_id, - channelId: post.channel_id, - previousRhsState: RHSStates.FLAG, - }; + const action = { + type: ActionTypes.SELECT_POST, + postId: post.root_id, + channelId: post.channel_id, + previousRhsState: RHSStates.FLAG, + timestamp: POST_CREATED_TIME, + }; - expect(store.getActions()[1]).toEqual(action); + expect(store.getActions()[1]).toEqual(action); + }); }); }); @@ -413,6 +420,7 @@ describe('rhs view actions', () => { type: ActionTypes.SELECT_POST, postId: '', channelId: '', + timestamp: 0, }, ])); diff --git a/components/create_comment/create_comment.jsx b/components/create_comment/create_comment.jsx index 41d5f42c5fbe..0ec24a090d0b 100644 --- a/components/create_comment/create_comment.jsx +++ b/components/create_comment/create_comment.jsx @@ -180,6 +180,11 @@ export default class CreateComment extends React.PureComponent { * To check if the timezones are enable on the server. */ isTimezoneEnabled: PropTypes.bool.isRequired, + + /** + * The last time, if any, when the selected post changed. Will be 0 if no post selected. + */ + selectedPostFocussedAt: PropTypes.number.isRequired, } static contextTypes = { @@ -251,7 +256,7 @@ export default class CreateComment extends React.PureComponent { this.scrollToBottom(); } - if (prevProps.rootId !== this.props.rootId) { + if (prevProps.rootId !== this.props.rootId || this.props.selectedPostFocussedAt > this.lastBlurAt) { this.focusTextbox(); } diff --git a/components/create_comment/create_comment.test.jsx b/components/create_comment/create_comment.test.jsx index 8cd47c865f35..a72c64422a1b 100644 --- a/components/create_comment/create_comment.test.jsx +++ b/components/create_comment/create_comment.test.jsx @@ -48,6 +48,7 @@ describe('components/CreateComment', () => { badConnection: false, getChannelTimezones: jest.fn(() => Promise.resolve([])), isTimezoneEnabled: false, + selectedPostFocussedAt: 0, }; test('should match snapshot, empty comment', () => { @@ -343,24 +344,79 @@ describe('components/CreateComment', () => { expect(showPostDeletedModal).toHaveBeenCalled(); }); - /* Removed due setProps not actually setting the props - * test('calls focusTextbox when rootId changes', () => { + describe('focusTextbox', () => { const draft = { message: 'Test message', uploadsInProgress: [1, 2, 3], fileInfos: [{id: '1', name: 'aaa', create_at: 100}, {id: '2', name: 'bbb', create_at: 200}], }; - const props = {...baseProps, draft}; - const wrapper = shallowWithIntl( - - ); + it('is called when rootId changes', () => { + const props = {...baseProps, draft}; + const wrapper = shallowWithIntl( + + ); - const focusTextbox = jest.fn(); - wrapper.instance().focusTextbox = focusTextbox; - wrapper.setProps({rootId: 'testid123'}); - expect(focusTextbox).toHaveBeenCalled(); - });*/ + const focusTextbox = jest.fn(); + wrapper.instance().focusTextbox = focusTextbox; + + const newProps = { + ...props, + rootId: 'testid123', + }; + + // Note that setProps doesn't actually trigger componentDidUpdate + wrapper.setProps(newProps); + wrapper.instance().componentDidUpdate(props, newProps); + expect(focusTextbox).toHaveBeenCalled(); + }); + + it('is not called when rootId does not change', () => { + const props = {...baseProps, draft}; + const wrapper = shallowWithIntl( + + ); + + const focusTextbox = jest.fn(); + wrapper.instance().focusTextbox = focusTextbox; + + // Note that setProps doesn't actually trigger componentDidUpdate + wrapper.setProps(props); + wrapper.instance().componentDidUpdate(props, props); + expect(focusTextbox).not.toHaveBeenCalled(); + }); + + it('is called when rootId does not change but the selectPostFocussedAt occurred after the last blur', () => { + const props = {...baseProps, draft, selectedPostFocussedAt: 1000}; + const wrapper = shallowWithIntl( + + ); + + const focusTextbox = jest.fn(); + wrapper.instance().focusTextbox = focusTextbox; + + // Note that setProps doesn't actually trigger componentDidUpdate + wrapper.setProps(props); + wrapper.instance().componentDidUpdate(props, props); + expect(focusTextbox).toHaveBeenCalled(); + }); + + it('is not called when rootId does not change and the selectPostFocussedAt occurred before the last blur', () => { + const props = {...baseProps, draft, selectedPostFocussedAt: 1000}; + const wrapper = shallowWithIntl( + + ); + + const focusTextbox = jest.fn(); + wrapper.instance().focusTextbox = focusTextbox; + wrapper.instance().handleBlur(); + + // Note that setProps doesn't actually trigger componentDidUpdate + wrapper.setProps(props); + wrapper.instance().componentDidUpdate(props, props); + expect(focusTextbox).not.toHaveBeenCalled(); + }); + }); test('handleChange should update comment draft correctly', () => { const onUpdateCommentDraft = jest.fn(); diff --git a/components/create_comment/index.js b/components/create_comment/index.js index b7db59b92219..e6c1e4071cdc 100644 --- a/components/create_comment/index.js +++ b/components/create_comment/index.js @@ -24,7 +24,7 @@ import { makeOnSubmit, makeOnEditLatestPost, } from 'actions/views/create_comment'; -import {getPostDraft, getIsRhsExpanded} from 'selectors/rhs'; +import {getPostDraft, getIsRhsExpanded, getSelectedPostFocussedAt} from 'selectors/rhs'; import CreateComment from './create_comment.jsx'; @@ -66,6 +66,7 @@ function makeMapStateToProps() { rhsExpanded: getIsRhsExpanded(state), badConnection, isTimezoneEnabled, + selectedPostFocussedAt: getSelectedPostFocussedAt(state), }; }; } diff --git a/reducers/views/rhs.js b/reducers/views/rhs.js index 54986d528e19..335d53f31d61 100644 --- a/reducers/views/rhs.js +++ b/reducers/views/rhs.js @@ -28,6 +28,17 @@ function selectedPostId(state = '', action) { } } +// selectedPostFocussedAt keeps track of the last time a post was selected, whether or not it +// is currently selected. +function selectedPostFocussedAt(state = 0, action) { + switch (action.type) { + case ActionTypes.SELECT_POST: + return action.timestamp || 0; + default: + return state; + } +} + function selectedPostCardId(state = '', action) { switch (action.type) { case ActionTypes.SELECT_POST_CARD: @@ -205,6 +216,7 @@ function isMenuOpen(state = false, action) { export default combineReducers({ selectedPostId, + selectedPostFocussedAt, selectedPostCardId, selectedChannelId, previousRhsState, diff --git a/reducers/views/rhs.test.js b/reducers/views/rhs.test.js index fae9ae5256f6..a47dc13c952a 100644 --- a/reducers/views/rhs.test.js +++ b/reducers/views/rhs.test.js @@ -9,6 +9,7 @@ import {ActionTypes, RHSStates} from 'utils/constants.jsx'; describe('Reducers.RHS', () => { const initialState = { selectedPostId: '', + selectedPostFocussedAt: 0, selectedPostCardId: '', selectedChannelId: '', previousRhsState: null, @@ -137,12 +138,14 @@ describe('Reducers.RHS', () => { type: ActionTypes.SELECT_POST, postId: '123', channelId: '321', + timestamp: 1234, } ); expect(nextState1).toEqual({ ...initialState, selectedPostId: '123', + selectedPostFocussedAt: 1234, selectedChannelId: '321', isSidebarOpen: true, }); @@ -155,12 +158,14 @@ describe('Reducers.RHS', () => { postId: '123', channelId: '321', previousRhsState: RHSStates.SEARCH, + timestamp: 4567, } ); expect(nextState2).toEqual({ ...initialState, selectedPostId: '123', + selectedPostFocussedAt: 4567, selectedChannelId: '321', previousRhsState: RHSStates.SEARCH, isSidebarOpen: true, @@ -175,12 +180,14 @@ describe('Reducers.RHS', () => { postId: '123', channelId: '321', previousRhsState: RHSStates.FLAG, + timestamp: 0, } ); expect(nextState3).toEqual({ ...initialState, selectedPostId: '123', + selectedPostFocussedAt: 0, selectedChannelId: '321', previousRhsState: RHSStates.FLAG, isSidebarOpen: true, diff --git a/selectors/rhs.jsx b/selectors/rhs.jsx index 83270de56e0c..a2945d517a1b 100644 --- a/selectors/rhs.jsx +++ b/selectors/rhs.jsx @@ -12,6 +12,10 @@ export function getSelectedPostId(state) { return state.views.rhs.selectedPostId; } +export function getSelectedPostFocussedAt(state) { + return state.views.rhs.selectedPostFocussedAt; +} + export function getSelectedPostCardId(state) { return state.views.rhs.selectedPostCardId; } diff --git a/selectors/rhs.test.js b/selectors/rhs.test.js index 79ee243bf2ac..7d9f032180f2 100644 --- a/selectors/rhs.test.js +++ b/selectors/rhs.test.js @@ -6,6 +6,18 @@ import assert from 'assert'; import * as Selectors from 'selectors/rhs.jsx'; describe('Selectors.Rhs', () => { + describe('should return the last time a post was selected', () => { + [0, 1000000, 2000000].forEach((expected) => { + it(`when open is ${expected}`, () => { + const state = {views: {rhs: { + selectedPostFocussedAt: expected, + }}}; + + assert.deepEqual(expected, Selectors.getSelectedPostFocussedAt(state)); + }); + }); + }); + describe('should return the open state of the sidebar', () => { [true, false].forEach((expected) => { it(`when open is ${expected}`, () => {