-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [2/n] cancel all -- notifications #976
feat: [2/n] cancel all -- notifications #976
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
bad6440
to
75bfa25
Compare
75bfa25
to
c9e3d9a
Compare
943f414
to
1a0b591
Compare
c9e3d9a
to
183f00f
Compare
183f00f
to
239ed7f
Compare
239ed7f
to
2916450
Compare
2916450
to
42e4994
Compare
42e4994
to
22cd1ff
Compare
@@ -680,6 +684,9 @@ export const notificationTypes: NotificationTypeConfig[] = [ | |||
const existingOrder = allOrders?.find((order) => order.id === localCancel.orderId); | |||
if (!existingOrder) return; | |||
|
|||
// skip if this is from a cancel all operation and isn't an error | |||
if (localCancel.isSubmittedThroughCancelAll && !localCancel.errorParams) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when user clicks cancel all, we won't show a new toast per cancel. we'll just have one notif, and only show a toast for the cancel when there was an error for that specific operation
isSubmittedThroughCancelAll?: boolean; | ||
}; | ||
|
||
export const CANCEL_ALL_ORDERS_KEY = 'all'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
canceling all orders in all markets, vs. just in the current market
@@ -157,7 +162,7 @@ export const accountSlice = createSlice({ | |||
: order | |||
) | |||
: state.localPlaceOrders, | |||
submittedCanceledOrders: hasNewFillUpdates | |||
localCancelOrders: hasNewFillUpdates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sad it was a typo from before
@@ -111,6 +115,7 @@ const initialState: AccountState = { | |||
historicalPnlPeriod: undefined, | |||
localPlaceOrders: [], | |||
localCancelOrders: [], | |||
localCancelAlls: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map of market (or 'all') -> cancel all objects
const isOrderCanceledByBackend = (orderId: string) => | ||
canceledOrderIdsInPayload.includes(orderId) && | ||
!state.localCancelOrders.map((order) => order.orderId).includes(orderId); | ||
const getNewCanceledOrderIds = (batch: LocalCancelAllData) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when there are new fill updates from indexer, we check for the newly canceled orders, and get the ones that are in the current local cancel all batch
22cd1ff
to
ecf23a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
light UX thought is that it seems really easy to hit "Cancel All" - do we want ot add an intermediate confirmation step (Are you sure you want to cancel all? modal)
action={ButtonAction.Primary} | ||
size={ButtonSize.XSmall} | ||
onClick={onClearOrCancelAll} | ||
isHighlighted={hasCancelableOrders} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think mostly to distinguish it from 'cancel all' and same with the faded X for dismiss -- but will sync with product/design for feedback!
notification, | ||
}: NotificationProps & ElementProps) => { | ||
const stringGetter = useStringGetter(); | ||
const isCancelForSingleMarket = localCancelAll.key !== CANCEL_ALL_ORDERS_KEY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit,. as a follow up maybe we could add some extra code to detect if all the orders are attributed to a single market, even if the user isn't filtered to it
: order | ||
); | ||
}, | ||
cancelAllSubmitted: ( | ||
state, | ||
action: PayloadAction<{ marketId?: string; orderIds: string[] }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, can we throw a comment explaining that not passing in a market id globally cancels all orders and passing in one only cancels orders for that market? I have a slight thought that it might also be clearer if we split it into cancelAllSubmitted
and cancelAllForMarketSubmitted
but I can also see that being a lil verbose
add notifications for cancel all orders
to test:
?cancelall=1
flag at the end of url