Search: Add sort order to search results

[codebykat]

Enhancement

This PR adds an ordering toggle to the bottom of the notes list when a search is active:

Will fix #1625

Test

1. Enter a search
2. Use the toggle to change the sort type and direction of the results.
3. Clear the search and verify that your global sort settings haven't been changed.
4. Start a new search and note that the sorting resets to the global setting.
5. Try in different browsers/OSes to make sure the dropdown styling is okay

NOTE: Title matches will get sorted to the top of the list as per #1705. So there's basically two separate sections: title and content. Within each section the notes should get sorted as per preferences. It's out of scope but I'd like to make this clearer in a separate PR.

Dark Mode screenshots

Release

RELEASE-NOTES.txt was updated in
932676d with:

Added sort order to search results #1726

[dmsnell]

@codebykat this is coming along and it looks like quite the challenge to figure out the details and ways that things are intertwined. good job and good luck on this journey!

[dmsnell]

This shouldn’t be necessary. Why is it here?

[dmsnell]

With two invocations right next to each other this function may not worth the extra abstraction it brings.

[dmsnell]

can we add new constants in the action-types file and follow the direction of Calypso? Why? The list of named action types gives us one place to see all the kinds of things we can do in the app and helps prevent us from creating duplicate actions; it makes it easier to analyze the code programmatically; we get free compiler errors if we try to use an action type that doesn't exist because webpack will complain when we import { NONEXISTENT_TYPE } from '../action-types'

[dmsnell]

here's a philosophical point of reflection: where is best to store the "toggle" state?

we can do it here in Redux middleware, which makes the UI components easier because they only need to call toggle(), but it complicates the Redux flows because the actions are no longer predictable.

we can do it in the UI and pass the next state, but that means we have to track it in the UI component and so complicates/duplicates it there.

one place I've come to accept is to use a hybrid of mapStateToProps() to get the existing state, mapDispatchToProps() to get the dispatch function, and occasionally, rarely, to use mergeProps() (also part of connect() but usually one we leave out) to keep the UI API clean and agnostic about the toggle state.

---

usually these complications are indications of a missing Eureka moment in a data structure somewhere. why is it hard for the UI to send the toggle state? (hint, boolean values should always raise our suspicion)

---

🤔 what if we stored sortDirection instead of isReversed?

export const toggleSearchDirectionFrom = direction => ( {
  type: SEARCH_SET_DIRECTION,
  direction: direction === 'normal' ? 'reversed' : 'normal',
} )

Manage it in the UI

not so bad because the state is simple enough and clear enough

toggleSearchDirection = () => this.props.toggleSearchDirection( this.props.searchDirection )

<button onClick={ this.toggleSearchDirection } />

const mapStateToProps = ( { search: { direction } } ) => ( {
  searchDirection: direction,
} )

const mapDispatchToProps = dispatch => ( {
  toggleSearchDirection: direction => dispatch( toggleSearchDirectionFrom( direction ) ),
} )

use mergeProps to remove the knowledge from the UI

more complicated connect() but more straightforward UI

<button onClick={ this.props.toggleSearchDirection } />

const mapStateToProps = ( { search: { direction } } ) => ( {
  searchDirection: direction,
} )

const mapDispatchToProps = dispatch => ( {
  setSearchDirection: direction => dispatch( toggleSearchDirectionFrom( direction ) ),
} )

const mergeProps = ( stateProps, dispatchProps, ownProps ) => ( {
  ...ownProps,
  ...stateProps,
  ...dispatchProps,
  toggleSearchDirection: () => dispatchProps.toggleSearchDirection( ownProps.searchDirection ),
} )

connect( mapStateToProps, mapDispatchToProps, mergeProps )( Component );

Of course, that looks scary, especially if unfamiliar with it. I'm a fan of tracking this in the UI until it's clear we need more abstraction. In any case, I still prefer keeping middleware behaviors outside of the action creators because of the pain I've had in trying to debug redux thunk stuff like this. In #1725 I recently proposed a new Redux middleware just for the purpose of executing the current search query (which is of course a bit more complicated because the search params are complicated). It's not that much more than Redux thunk actions but it preserves the predictability and audibility of our action sequence. [adding middleware, defining middleware]

Middleware for predictability and introspection

export const toggleSortOrder = () => ( {
  type: SORT_ORDER_TOGGLE,
} );

export const sortDirection = ( state = 'normal', { type, direction } ) =>
  SORT_ORDER_SET === 

Eureka!

See? it was way simpler than we were realizing. This is best handled in the reducer where all the data lives.

export const sortOrder = ( state = 'normal', { type } ) =>
  SORT_ORDER_TOGGLE === type
    ? ( state === 'normal' ? 'reverse' : 'normal' )
    : state;

export toggleSortOrder = () => ( {
  type: SORT_ORDER_TOGGLE,
} );

<button onClick={ this.props.toggleSortOrder } />

const mapStateToProps = ( { search: { sortOrder } } ) => ( {
  sortOrder,
} )

const mapDispatchToProps = ( {
  toggleSortOrder
} )

[dmsnell]

we would have good value in using a new reducer in a search directory where we can store all the filtering and sort properties. these reducers used to be separate but they were merged and that can cause issues, especially with regards to handling more than one action type in a given reducer. it might be worth our time to decouple these as they once were but that would be a job for another PR

you can see what I was exploring in https://github.com/Automattic/simplenote-electron/pull/1725/files#diff-a36f9dcd572ee4de0417fd7a1f181542

[SylvesterWilmott]

Looks good!

We can an icon we can use for the reordering results in the new icon pack

[dmsnell]

Why do we have local and global search orders? Would it make sense to only have a single global sort order that changes when we click to change it?

That would simplify the amount of code and complexity we have to introduce to support both sorting systems

[dmsnell]

If we leave this check for the calling parent component then it will clean up this function and we can remove the <Fragment>

It means moving the check to the parent component but at least then what it leaves us is a component that can stand on its own a bit better.

[codebykat]

Closing in favor of #2542