React/redux Task Search #1121
React/redux Task Search #1121
Conversation
| return { | ||
| url: `/history/request/${requestId}/tasks?count=${count}&page=${page}${params.join('')}` | ||
| } | ||
| }); |
There was a problem hiding this comment.
can we update this to match the API layout that @wolfd rolled out?
| if (orderDirection) params.push(`&orderDirection=${orderDirection}`); | ||
| return { | ||
| url: `/history/tasks?count=${count}&page=${page}${params.join('')}` | ||
| }; |
There was a problem hiding this comment.
we may want to use a function for building query params (like $.param(), but I'd rather not depend on jQuery), since it will handle escaping, etc. for us
|
@Calvinp Deleted |
|
|
Found another file you can delete: LinkedFormItem |
| }, | ||
| disableNext: false, | ||
| loading: false | ||
| }; |
There was a problem hiding this comment.
would be preferable to have this in redux
There was a problem hiding this comment.
I can see the argument for having this in redux, but we should think about where we draw the line between component state and redux state.
I'd argue that since this doesn't effect anything besides the state of this component to leave it in. Especially since the state of the underlying form is already in the store via redux-form. I'm open to conversation though.
There was a problem hiding this comment.
Long term, task search results should be true permalinks (i.e. have all this state data in the URL), and that would make for a good argument to move this stuff in to Redux (I'm using reduxjs/redux#1287 (comment) as a guide). But since permalinks don't exist in the original implementation, I'm okay with circling back on this.
There was a problem hiding this comment.
In the context of permalinks, that does make sense. That will be easier once this is in react router as well. Sounds like a plan 👍
|
If anyone here see any reason why these should two branches not be united, speak now or forever hold thy peace. |
:partyparrot:
@tpetr @wolfd @Calvinp