-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Completed orders should break items out by Shop #2645
Conversation
@spencern As long as that last tiny css change is in there. I'm good yes. (see my previous comment) |
One small thing and one big thing. Big: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not seen this issue with adding products to the cart before, so I'm inclined to believe that it's with this branch.
The shipping name is a design question and not a blocker.
@zenweasel @spencern Totally agree that the Name should be in the shipping box. Good catch. I added to my zeplin mocks. |
@rymorgan "Quantity Total" feels like a really weird way to say the total number of items in an order. I realize that it's itemized in the box to the left, which helps, but it still feels weird to me. |
@spencern I don't disagree, I took that from what was there because @aaronjudd wanted me to use the existing language. I think 'Quantity' would work though. |
@spencern I'm not able to replicate the "Cart Empty" issue with either a single shop or multi-shop checkout. Is there something specific I should be doing? I'll keep trying though |
Ok, I was finally able to reproduce the bug and it's something that I fixed a long time ago in the Blaze version with this code:
It seems to be caused by the fact that we delete that Cart record and for some reason the subscription doesn't update (possibly the OpLog is tied to that original ID). So stopping and resubscribing used to fix it. However, putting this same code in the container does not seem to resolve this issue as the new subscription is not going into place. Continuing to try and see how I can fix this. Super painful because it only happens the first time after a full reset. |
@spencern Ok, so that bug should be fixed now |
@rymorgan @zenweasel re: |
@rymorgan and where you got it from is inconsistent.. |
@aaronjudd is this language a blocker for you here or should I merge and create a new issue for updating copy? |
@spencern let's create an issue for label consistency, and we can tackle as one last design review before we call release the first marketplace. I'll give this full reviewing, ignoring labels in 10.. |
@spencern @aaronjudd -- I'll go through all my zeplin docs and update the label to "Items in cart" and also update everything to sentence case. |
Closes #2308 and #2245
My first React component replacement from scratch so I open to input on layout, structure, etc. Layout is responsive thanks to help from @kieckhafer. Also first time I have done any HTML of significance so there might be problems there.
To Test
Single Shop (for just the completed page functionality)
Multi-shop
[Update] Slightly shorter instructions now that you can see products from both shops w/o switching shops.
reaction.json
)**Note that "Estimated Ship Date" is not implemented on the back end so that's why it doesn't display here.
Another Note There is currently an issue with the way that discounts are being stored on multi-shop orders so that discounts are doubled (once per shop).