search

eltrino / magento2-hyva-checkout-stripe

5 stars 2 forks source link

Improvements needed #2

Closed rajeev-k-tomy closed 2 years ago

rajeev-k-tomy commented 2 years ago
  1. We are collecting dispatch value of AppContext through the custom hook src/hooks/useAppContext. However, using dispatch directly is vague and confusing as it clearly does not tell which dispatch it is. Maybe using appDispatch make much more sense here.
  2. It is good to rename hooks below. This way it will be clear the custom hooks are local to stripe payment:
    • useAppContext => useStripeAppContext
    • useCartContext => useStripeCartContext
    • useCheckoutFormContext => useStripCheckoutFormContext
  3. place order method in src/hooks/useStripePayments.js is not passing all the dependencies into useCallback
  4. Dependencies not specified in useEffect here https://github.com/eltrino/magento2-hyva-checkout-stripe/blob/main/src/components/Cards.jsx#L21-#L28
odi-um commented 2 years ago

hi @rajeev-k-tomy, thank you for your input, but i'm not sure i'm following you regarding first item. do you mean that its not obvious bc of naming or i need to use src/hooks/useAppContextinstead of direct call ofuseContext(AppContext) ?

rajeev-k-tomy commented 2 years ago

@odi-um No, the dispatch variable is confusing. Because we have dispatch available for CartContext too. Basically, we have two dispatches. So specifying the dispatch variable either as appDispatch or cartDispatch would be much more cleaner naming in my opinion. In that way, it is clear which dispatch is referring in the code.

odi-um commented 2 years ago

thanks for clarification

odi-um commented 2 years ago

@rajeev-k-tomy could you check pr #3? have i covered everything ?