vendure-ecommerce / storefront-remix-starter

A storefront starter kit for Vendure built with Remix
https://remix-storefront.vendure.io
176 stars 93 forks source link

Add labels and mutations for default addresses #39

Closed DanielBiegler closed 1 year ago

DanielBiegler commented 1 year ago

This contains commits from #37 and #38 - merge those first. There are no other PRs open so I assumed we'll just merge them in a row so it all should work out. TODO: mutations for setting.

Header Header
image image
netlify[bot] commented 1 year ago

Deploy Preview for inspiring-boba-355f4d ready!

Name Link
Latest commit 6de1a445965310708862b2c883656fb5cf6bd6ec
Latest deploy log https://app.netlify.com/sites/inspiring-boba-355f4d/deploys/6443ebeb92c422000840ae6c
Deploy Preview https://deploy-preview-39--inspiring-boba-355f4d.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] commented 1 year ago

Deploy Preview for remix-vendure ready!

Name Link
Latest commit 6de1a445965310708862b2c883656fb5cf6bd6ec
Latest deploy log https://app.netlify.com/sites/remix-vendure/deploys/6443ebeb2c40f90008b20b75
Deploy Preview https://deploy-preview-39--remix-vendure.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

kyunal commented 1 year ago

Just FYI before you continue, same conflicts here 😅

DanielBiegler commented 1 year ago

All good, thats to be expected, no problem :smile:

DanielBiegler commented 1 year ago

Fixed it now but appearently Github doesnt reset the merged commits automatically so I'll try the solution from here. It says to change the base branch back and forth. Lets try it.

kyunal commented 1 year ago

That seems to have done the trick, GitHub doesn't complain to me about conflicts anymore

DanielBiegler commented 1 year ago

I just found out that by dismissing the address deletion modal, you actually confirm it and delete the address, that made me laugh

Screencast from 21.04.2023 16:42:05.webm

Let's fix this too as part of this PR :laughing:

DanielBiegler commented 1 year ago

@kyunal @michaelbromley we got customer address handling now :D

Screencast from 22.04.2023 16:17:51.webm

I pretty much completely reworked the address page because it didnt even use Remix' patterns and forms. This video by Remix' maker Ryan Florence helped me implement this. The form pattern works really nicely.

kyunal commented 1 year ago

This mostly looks good to me. Just for my own understanding and for updating the README accordingly - this is only for setting the default shipping/billing address, but it isn't used in checkout, right? I'm still happy to merge without, just want it to be documented correctly.

Separate billing addresses aren't supported by the checkout at all right now and if I select a default shipping address it isn't automatically selected.

DanielBiegler commented 1 year ago

Correct. I implemented it at EditAddressCard which is only being used in the addresses route, nowhere else. The checkout uses ShippingAddressSelector which I havent taken a look at yet. I'd prefer doing that in a new PR to keep it shorter and easier to oversee.

I guess having the whole CRUD actions covered is nice UX, then new customers dont have to switch routes i.e. Creating, updating, deleting and setting default addresses in the checkout covers what we need. Anything else?

kyunal commented 1 year ago

I think that's about it. Some of the actions might even be overkill for checkout, e.g. setting default addresses could make everything be very crowded. But that shouldn't mean we should rule that out entirely, just if it comes down to it it's fine to disregard certain features for checkout.

And yes, it makes sense to separate them. Again, thanks for all the work you've recently put into this starter. I'll go ahead and merge this.