Skip to content

Add labels and mutations for default addresses #39

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

Merged

Conversation

DanielBiegler
Copy link
Contributor

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

we dont need it if theres no label
sometimes you dont want it, example: pagination
used this for testing line clamping - it worked
UI is not connected to pagination yet, testing via url params works though
this makes the focus ring fully visible for button groups
- take/skip pagination works now
- add pagination label
- refine zod validation schema
- redirect to searchless url
- filter active orders out
- new error msg for when theres no more orders due to pagination
@netlify
Copy link

netlify bot commented Apr 20, 2023

Deploy Preview for inspiring-boba-355f4d ready!

Name Link
🔨 Latest commit 6de1a44
🔍 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
Copy link

netlify bot commented Apr 20, 2023

Deploy Preview for remix-vendure ready!

Name Link
🔨 Latest commit 6de1a44
🔍 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
Copy link
Collaborator

kyunal commented Apr 21, 2023

Just FYI before you continue, same conflicts here 😅

@DanielBiegler
Copy link
Contributor Author

All good, thats to be expected, no problem 😄

@DanielBiegler
Copy link
Contributor Author

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.

@DanielBiegler DanielBiegler changed the base branch from master to cf-pages April 21, 2023 07:48
@DanielBiegler DanielBiegler changed the base branch from cf-pages to master April 21, 2023 07:48
@kyunal
Copy link
Collaborator

kyunal commented Apr 21, 2023

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

@DanielBiegler
Copy link
Contributor Author

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 😆

@DanielBiegler
Copy link
Contributor Author

DanielBiegler commented Apr 22, 2023

@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.

@DanielBiegler DanielBiegler marked this pull request as ready for review April 22, 2023 14:20
@kyunal
Copy link
Collaborator

kyunal commented Apr 24, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

kyunal commented Apr 24, 2023

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.

@kyunal kyunal merged commit b876b3b into vendure-ecommerce:master Apr 24, 2023
@DanielBiegler DanielBiegler deleted the feature/default-addresses branch April 24, 2023 10:27
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.

2 participants