[project-base][SSP-2096] order process refactoring #3155
Conversation
sebaholesz
added
the
DX & Refactoring
sebaholesz
force-pushed
the
sh-ssp-2096-order-process-refactoring
branch
11 times, most recently
from
89a19e3
to
1c0ce36
Compare
sebaholesz
force-pushed
the
sh-ssp-2096-order-process-refactoring
branch
7 times, most recently
from
363ed66
to
41413e4
Compare
sebaholesz
changed the title
sebaholesz
force-pushed
the
sh-ssp-2096-order-process-refactoring
branch
from
07aeefe
to
9a26c79
Compare
- it now clearly defines the cart object - it now returns isCartFetchingOrUnavailable instead of isFetching, which is also based on auth loading and cart not being undefined
- renamed various variables - simplified PromoCode component in order for the UI to be more isolated from the functionality
- they now allow fo displaying of skeletons even during initial load, as the skeleton manager was included in all 3 order process pages - going back and forth in order process is now only done using callbacks (not links), as we now always start page-loading on navigation
… to fill a new delivery address
…ress if filled for the second time - this ensures better testing, as it checks if the address is correctly modified
sebaholesz
force-pushed
the
sh-ssp-2096-order-process-refactoring
branch
from
9a26c79
to
dc3cb37
Compare
| ) => { | ||
| const { t } = useTranslation(); | ||
| const updatePortalContent = useSessionStore((s) => s.updatePortalContent); | ||
| const promoCodeValidationMessages = useMemo(() => { |
There was a problem hiding this comment.
I wonder why we don't use react-hook-form and yup there.
I understand this seems like overkill here, but if the project wants to extend the validation or something, it wouldn't be really comfortable.
For example, the newsletter form uses yup and also doesn't have many fields
| const goToPreviousStepFromCartPage = () => { | ||
| updatePageLoadingState({ isPageLoading: true, redirectPageType: 'homepage' }); | ||
| router.push('/'); | ||
| }; |
There was a problem hiding this comment.
I'm not sure if redirecting to the homepage is valid solution. Inspiring by projects, usually this button calls history.back()
| formMeta: ContactInformationFormMetaType, | ||
| ) => { | ||
| const { t } = useTranslation(); | ||
| const [{ fetching }, createOrderMutation] = useCreateOrderMutation(); |
There was a problem hiding this comment.
Fetching is too general name, please rename it to a more specific name
There was a problem hiding this comment.
Name of the file is too general, please rename it to a more specific name.
I will be more comfortable to search this file
| currentCart.transport && | ||
| currentCart.payment | ||
| ) { | ||
| const query: OrderConfirmationQuery = { |
There was a problem hiding this comment.
Query is too general name, please rename it to a more specific name.
Firstly i thought that it's query for graphql
There was a problem hiding this comment.
Please use more specific name for the file
There was a problem hiding this comment.
That's a general comment.
I would like to have unit tests for these utils files, it brings a lot of code stability.
I implemented it on breno and it paid off 100%
| const { orderUuid, orderPaymentType } = query as OrderConfirmationQuery; | ||
|
|
||
| const gtmStaticPageViewEvent = useGtmStaticPageViewEvent(GtmPageType.order_confirmation); | ||
| useGtmPageViewEvent(gtmStaticPageViewEvent); | ||
|
|
||
| const [{ data: orderSentPageContentData, fetching }] = useOrderSentPageContentQuery({ | ||
| const [{ data: orderSentPageContentData, fetching: isFetching }] = useOrderSentPageContentQuery({ |
There was a problem hiding this comment.
Please rename isFetching to a more specific name, it would help to read the code somewhere below
| @@ -9,6 +9,9 @@ const CUSTOM_PAGE_TYPES = [ | |||
| 'orders', | |||
| 'order', | |||
| 'productMainVariant', | |||
| 'order-process', | |||
There was a problem hiding this comment.
On the projects we will want to have separately skeleton for transport and payment and contact information.
So let's implement that's on platform first
Or what's your opinion on it?
| const isWithCart = isUserLoggedIn || !!cartUuid; | ||
|
|
||
| useEffect(() => { | ||
| updatePageLoadingState({ isCartHydrated: true }); | ||
| }, []); | ||
|
|
||
| const [{ data: fetchedCartData, fetching }, fetchCart] = useCartQuery({ | ||
| const [{ data: fetchedCartData, fetching: isFetching }, fetchCart] = useCartQuery({ |
There was a problem hiding this comment.
Same there isFetching too general variable for me 🤔
🌐 Live Preview: