Refactor: Replace search field focus state with middleware #1914
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Alternative idea to #1908 `searchFocus` isn't quite app state. We have been using it as a workaround in order to trigger side-effects from Redux in order to focus the search field. In this patch we're removing that state altogether and replacing it with a custom middleware specific to the search field. This too is somewhat ugly because of the coupling between the middleware and the search field component; it needs more thought on how best to register the DOM node containing focus. Nevertheless by removing the `state` property we are able to remove more code and the update loop that was necessary to set focus, update, an then record that the focus was already set or unset. Storing a state value for focus doesn't match the reality that the focus is a property of the DOM node and changes independently from the Redux actions being dispatched in the app.
codebykat
approved these changes
Feb 19, 2020
There was a problem hiding this comment.
I like this. It's less of a hack than the existing code (or my PR), works fine, and incidentally fixes a minor interaction bug (after selecting a suggested tag, the cursor is placed in the search field, but the text is now not selected, which is as it should be IMO).
belcherj
reviewed
Feb 19, 2020
| @@ -18,20 +18,33 @@ export class SearchField extends Component<ConnectedProps> { | |||
|
|
|||
| inputField = createRef<HTMLInputElement>(); | |||
|
|
|||
| componentDidUpdate() { | |||
| const { searchFocus, onSearchFocused } = this.props; | |||
| componentDidMount() { | |||
There was a problem hiding this comment.
I think we are making a mistake adding more componentDidMounts to the app, at very least this should be: UNSAFE_componentWillMount
There was a problem hiding this comment.
That being said, let's merge this.
belcherj
deleted the
refactor/replace-search-focus-state-with-middleware
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative idea to #1908
searchFocusisn't quite app state. We have been using it as aworkaround in order to trigger side-effects from Redux in order
to focus the search field.
In this patch we're removing that state altogether and replacing
it with a custom middleware specific to the search field. This too
is somewhat ugly because of the coupling between the middleware
and the search field component; it needs more thought on how best
to register the DOM node containing focus.
Nevertheless by removing the
stateproperty we are able to removemore code and the update loop that was necessary to set focus,
update, an then record that the focus was already set or unset.
Storing a state value for focus doesn't match the reality that the
focus is a property of the DOM node and changes independently from
the Redux actions being dispatched in the app.