Reorder transactions of same date #1603
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded
Removed
Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
joel-jeremy
force-pushed
the
sortable-transactions
branch
from
866571b to
96945da
Compare
| @@ -28,6 +28,8 @@ export interface ServerHandlers { | |||
|
|
|||
| 'transaction-delete': (transaction) => Promise<EmptyObject>; | |||
|
|
|||
| 'transaction-move': (arg: { id; accountId; targetId }) => Promise<unknown>; | |||
There was a problem hiding this comment.
🥜 nitpick: ideally we shouldn't use unknown and any types for new code we add.
| if ( | ||
| query.state.orderExpressions && | ||
| query.state.orderExpressions.length > 0 | ||
| ) { | ||
| // If there are no order expressions in the query, | ||
| // transactions are sorted by sort_order by default. | ||
| // In case there are order expressions, let's add back | ||
| // the sort_order sorting so that transactions still | ||
| // maintain the correct order. | ||
| query = query.orderBy({ sort_order: 'desc' }); | ||
| } |
There was a problem hiding this comment.
❓ question: should this logic also be added to mobile account component? And if yes: any way we could unify it so we don't have duplicate sorting logic?
(I don't mean actually implementing FULL drag-n-drop support on mobile; just that the list is ordered the same order on both desktop and mobile)
| accountId: trans.account, | ||
| targetId: sort.targetId, | ||
| }); | ||
| onRefetch(); |
There was a problem hiding this comment.
❓ question: why do we need to refetch after sorting?
| onExpose={name => !isPreview && onEdit(id, name)} | ||
| onUpdate={value => { | ||
| onUpdate('date', value); | ||
| }} | ||
| unexposedContent={ | ||
| <View innerRef={dragRef}> |
There was a problem hiding this comment.
💬 suggestion: adding drag handler to only the date column makes the functionality pretty hidden. What do you think about adding a drag-handler at the beginning of the row?
For example: this is what gmail used to do
| const lastTransaction = await first( | ||
| 'SELECT sort_order FROM v_transactions ' + | ||
| 'WHERE date = ? AND is_child <> 1 AND parent_id IS NULL ' + | ||
| 'ORDER BY sort_order DESC LIMIT 1', | ||
| [transaction.date.replace(/-/g, '')], // Remove hyphens | ||
| ); |
There was a problem hiding this comment.
💭 thought: this could potentially be a pretty expensive SQL query. Especially for bigger databases.
Do we really need the last transactions sort_order when creating a new transaction? Could we just set it to 99999 or something like that? And then change sorting from sort_by:sort_order to sort_by:sort_order,insertion_id (i.e. two index sorting).
WDYT?
| 'SELECT vt.id, vt.sort_order FROM v_transactions vt WHERE vt.account = ? ' + | ||
| 'AND vt.date = (SELECT vt2.date FROM v_transactions vt2 WHERE vt2.id = ? LIMIT 1) ORDER BY sort_order', |
There was a problem hiding this comment.
🥜 nitpick: if you use ` symbol - you can write multi line strings.
|
This is awesome, it'll definitely be really useful to be able to drag transactions around. I thought of two enhancements, but these likely should be in a future PR/issue:
|
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
I agree that it would be nice to not show the blue lines on places that the transaction cant be dropped. Im not sure if a visual indicator is needed considering that we expect people to find the reorder functionality on the budget page without an indicator. It works pretty similar where only the far left item can be grabbed to move the line. |
|
@joel-jeremy What is left on this? |
joel-jeremy
force-pushed
the
sortable-transactions
branch
from
96945da to
50e63ea
Compare
|
Would it be possible to make it work on the "All accounts" view as well? |
joel-jeremy
force-pushed
the
sortable-transactions
branch
from
50e63ea to
5c8ebff
Compare
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR was closed because it has been stalled for 5 days with no activity. |
Resolves #1165