Address actions #2308
Address actions #2308
Conversation
- add delete functionality for address entries - move isUploadingData state to parent component - refactor child components to use shared loading state
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ne address form types - Refactored translation key to use ICU select pattern for dynamic translations. - use useMemo to find primary, billing, address - Defined constant ADDRESS_FORM_TYPE for address form actions (add, edit) and associated type.
…ze modal rendering with useMemo and useCallback"
… to the root component
There was a problem hiding this comment.
@sunilsabatp Mostly minor feedback, except for the state management of addressAction
| "deleteAddress": "Delete Address", | ||
| "delete": "Delete", | ||
| "addressConfirmationMessage": "Are you sure you want to set this address as your {addressType, select, mailing {billing} other {{addressType}}} address? {isAddressSet, select, true {This will replace your current {addressType, select, mailing {billing} other {{addressType}}} address.} other {}}", | ||
| "deleteAddressConfirmationMessage": "Are you sure you want to delete this address? If you want to use it again, please add it as a new address.", | ||
| "confirm": "Confirm", | ||
| "address2": "Address 2(optional)", |
There was a problem hiding this comment.
Do some of the keys that are currently outside need to be moved/grouped?
For example, I see delete, and address2 keys
| @@ -147,6 +147,12 @@ | |||
| "setAsPrimaryAddress": "Set as Primary Address", | |||
| "setAsBillingAddress": "Set as Billing Address" | |||
| }, | |||
| "deleteAddress": "Delete Address", | |||
| "delete": "Delete", | |||
| "addressConfirmationMessage": "Are you sure you want to set this address as your {addressType, select, mailing {billing} other {{addressType}}} address? {isAddressSet, select, true {This will replace your current {addressType, select, mailing {billing} other {{addressType}}} address.} other {}}", | |||
There was a problem hiding this comment.
While this works, I think it might be difficult for a translator to understand.
Can the interpolation be made simpler?
For example, we could directly pass in the translated text (e.g. "primary address") for the address type in a single variable and use where needed.
| <AddressDeleteModal | ||
| addressId={selectedAddressForAction?.id} | ||
| setIsModalOpen={setIsModalOpen} | ||
| fetchUserAddresses={fetchUserAddresses} | ||
| /> | ||
| ); | ||
| case ADDRESS_ACTIONS.SET_PRIMARY: | ||
| return ( | ||
| <AddressTypeConfirmationModal | ||
| addressType={ADDRESS_TYPE.PRIMARY} | ||
| userAddress={primaryAddress} | ||
| {...addrTypeConfProps} | ||
| /> | ||
| ); | ||
| case ADDRESS_ACTIONS.SET_BILLING: | ||
| return ( | ||
| <AddressTypeConfirmationModal | ||
| addressType={ADDRESS_TYPE.MAILING} | ||
| userAddress={billingAddress} | ||
| {...addrTypeConfProps} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
- Since
AddressDeleteModalandAddressTypeConfirmationModalrepresent the content in the modal instead of the modal itself, rename accordingly (similar to AddAddress and EditAddress). AddressTypeConfirmationModal- wouldUpdateAddressTypebe a more readable/understandable name?
| const fetchUserAddresses = useCallback(async () => { | ||
| if (!user || !token || !contextLoaded) return; |
There was a problem hiding this comment.
Could you rename fetchUserAddresses to updateUserAddresses, as this does the job of both fetching and updating the addresses.
There was a problem hiding this comment.
Important
There are some issues in state management.
While the addressAction state is updated while opening the modal, it is not updated while closing the modal. So if I edit an address successfully, the addressAction remains edit. The same applies if I click on the Cancel button - isModalOpen is updated to false, but addressAction remains unchanged.
| const addrTypeConfProps = { | ||
| setIsModalOpen, |
There was a problem hiding this comment.
- Avoid abbreviations unless absolutely necessary. This should become shorter if you rename
AddressTypeConfirmationModaltoUpdateAddressType - Since these are common props, I suggest keeping that in the name. You could also simply add the props in the component directly, instead of destructuring. There aren't many, so it should not impact readability much.
| } | ||
| }; | ||
| return ( | ||
| <div className={styles.addrConfirmContainer}> |
There was a problem hiding this comment.
I don't think addrConfirmContainer needs to be abbreviated.
There was a problem hiding this comment.
Could you clean up this scss file? Spacing between classes is erratic
There was a problem hiding this comment.
I'll cover in this #2312
This PR handle the address actions (
delete, setPrimary, setBilling).