Skip to content
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

Improved: Order detail page on the open tab (#283) #302

Merged
merged 27 commits into from
Nov 1, 2023

Conversation

sanskar345
Copy link
Contributor

Related Issues

Closes #283

Short Description and Why It's Useful

  • Order detail page enhancement for desktop and mobile view on the open tab.

Screenshots of Visual Changes before/after (If There Are Any)

In desktop -

Before -
page before desk 1
page before desk 2

After -
page afterrrr

In mobile

Before -
page before mob 1

page before mob 2

After -

page after mob

Order Rejected Item History Modal -

In desktop -
histt desk

In mobile -
histt mob

Report An Issue Modal -

In desktop -
reporttt desk

In mobile -
reportt mob

Contribution and Currently Important Rules Acceptance

@sanskar345 sanskar345 requested a review from dt2patel September 27, 2023 13:46
Copy link
Contributor

@dt2patel dt2patel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments @sanskar345

@@ -47,6 +47,8 @@
"Instance Url": "URL de la instancia",
"In store": "En tienda",
"Inventory": "Inventario",
"If you cannot fulfill this order, will be sent an email and the order item will be removed from your dashboard.": "If you cannot fulfill this order, { customerName } will be sent an email and the order item will be removed from your dashboard.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this line is accurate @Dhiraj1405

@@ -76,6 +76,23 @@ http://ionicframework.com/docs/theming/ */
--ion-color-light-tint: #f5f6f9;
}

.empty-state {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As sir, these styles need to be global as we can have multiple empty states in the app.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but I mean why on this section of the file?

Copy link
Contributor Author

@sanskar345 sanskar345 Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sir, I thought these styles should be coded at last but before the media query, as the media query is written at last I think.

should I add thses at last??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check where the other custom styles are? I would think they should be grouped together in some kind of sequence. Try that @sanskar345

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sir done.

@@ -0,0 +1,132 @@
<template>
<section>
<ion-item color="light" lines="none">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong component

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sir, any suggestion regarding this?

</ion-button>
</ion-item>
<main :class="{ 'desktop-only' : isDesktop }">
<OrderInfo v-if="!isDesktop" :orderId="$route.params.orderId" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to do this without using order info component twice?

Copy link
Contributor Author

@sanskar345 sanskar345 Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sir, previously I have tried this approach - 428740b

video of UI -

UI.changes.webm

…n order detail page and removed unnecessary entries in locale files (hotwax#283)
@sanskar345 sanskar345 requested a review from dt2patel September 29, 2023 12:29
Copy link
Contributor

@dt2patel dt2patel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

grid-area: right;
/* Order Info section to stay fixed and not scrollable as there can be multiple order items which will increase the grid container size */
height: fit-content;
position: sticky;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't need to be sticky. That will fix your height problem too. Try that to simplify the code

Copy link
Contributor Author

@sanskar345 sanskar345 Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screencast.from.04-10-23.06.58.07.PM.IST.webm

@dt2patel Sir added sticky because we want order Information to be fixed and the user can scroll through the order items.

Added height because it was taking max height of the grid container

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I understand that but it is better for user to scroll the entire page here instead of both elements having different behaviors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sir.

}

aside {
grid-area: right;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
grid-area: right;
grid-column: 2;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside {
    grid-column: 2;
    grid-row: 1;
}

I also have to use the grid-row. If I don't, it places the section (order items) in the second row first col.

For css:

aside {
   grid-column: 2;
}

Screenshot from 2023-10-05 11-23-27

For css:

aside {
    grid-column: 2;
    grid-row: 1;
}

Screenshot from 2023-10-05 11-22-55

main {
display: grid;
grid-template-columns: 1fr 1fr;
grid-template-areas: 'left right';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
grid-template-areas: 'left right';

grid-template-columns: 1fr 1fr;
grid-template-areas: 'left right';
gap: var(--spacer-base);
margin: var(--spacer-lg) var(--spacer-lg) 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
margin: var(--spacer-lg) var(--spacer-lg) 0;

Comment on lines 302 to 304
section {
grid-area: left;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
section {
grid-area: left;
}

@sanskar345 sanskar345 requested a review from dt2patel October 5, 2023 06:02
src/store/modules/util/actions.ts Outdated Show resolved Hide resolved
</ion-toolbar>
</ion-header>
<ion-content>
<ion-list v-for="(history, index) in rejectionHistory" :key="index">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to define for loop on the ion-item instead of defining it on ion-list

@@ -470,7 +473,7 @@ const actions: ActionTree<OrderState , RootState> ={
return await dispatch("rejectOrderItems", payload).then((resp) => {
const refreshPickupOrders = resp.find((response: any) => !(response.data._ERROR_MESSAGE_ || response.data._ERROR_MESSAGE_LIST_))
if (refreshPickupOrders) {
showToast(translate('All items were canceled from the order') + ' ' + payload.orderId);
showToast(payload.part.items.length === 1 ? translate('Item has been rejected successfully') : translate('All items were canceled from the order') + ' ' + payload.orderId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
showToast(payload.part.items.length === 1 ? translate('Item has been rejected successfully') : translate('All items were canceled from the order') + ' ' + payload.orderId);
showToast(payload.part.items.length === 1 ? translate('Item has been rejected successfully') : translate('All items were rejected from the order') + ' ' + payload.orderId);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check on this condition.

@@ -450,6 +450,9 @@ const actions: ActionTree<OrderState , RootState> ={
}
})
}
// Mark current order as ready to handover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comment that why we need readyToHandover property on order

Comment on lines 35 to 51
alertController,
IonButton,
IonButtons,
IonContent,
IonFab,
IonFabButton,
IonHeader,
IonIcon,
IonItem,
IonLabel,
IonList,
IonListHeader,
IonTitle,
IonToolbar,
modalController,
IonRadio,
IonRadioGroup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
alertController,
IonButton,
IonButtons,
IonContent,
IonFab,
IonFabButton,
IonHeader,
IonIcon,
IonItem,
IonLabel,
IonList,
IonListHeader,
IonTitle,
IonToolbar,
modalController,
IonRadio,
IonRadioGroup
IonButton,
IonButtons,
IonContent,
IonFab,
IonFabButton,
IonHeader,
IonIcon,
IonItem,
IonLabel,
IonList,
IonListHeader,
IonTitle,
IonToolbar,
IonRadio,
IonRadioGroup,
alertController,
modalController,

<ion-item lines="none">
<aside>
<ion-item v-if="order?.readyToHandover || order?.rejected" color="light" lines="none">
<ion-icon :icon="order?.readyToHandover ? checkmarkCircleOutline : order?.rejected ? closeCircleOutline : ''" :color="order?.readyToHandover ? 'success' : order?.rejected ? 'danger' : ''" slot="start" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to have the extra check for order.rejected in icon ternary condition as this block will only be displayed when we have readyToHandover or rejected order.
Also, conditional chaining is also not required inside the ion-item.

<aside>
<ion-item v-if="order?.readyToHandover || order?.rejected" color="light" lines="none">
<ion-icon :icon="order?.readyToHandover ? checkmarkCircleOutline : order?.rejected ? closeCircleOutline : ''" :color="order?.readyToHandover ? 'success' : order?.rejected ? 'danger' : ''" slot="start" />
<ion-label class="ion-text-wrap">{{ order?.readyToHandover ? $t("Order is now ready to handover.") : order?.rejected ? $t("Order has been rejected.") : '' }}</ion-label>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above comment.

<h2>{{ order.customer?.name }}</h2>
<p>{{ order.orderName ? order.orderName : order.orderId }}</p>
<p class="overline">{{ $t("Handling Instructions") }}</p>
<p>{{ order?.shippingInstructions }}</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p>{{ order?.shippingInstructions }}</p>
<p>{{ order.shippingInstructions }}</p>

@sanskar345
Copy link
Contributor Author

sanskar345 commented Oct 9, 2023

@dt2patel

New UI -

In Desktop:
UI desk.webm

In Mobile:
UI mob.webm

@ravilodhi ravilodhi merged commit 315ffe0 into hotwax:main Nov 1, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance Order Details Page on the Open Tab
4 participants