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
Enhancement: Add order note to display held stock inventory. #29132 #37833
Enhancement: Add order note to display held stock inventory. #29132 #37833
Conversation
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.
Hi @Abdalsalaam!
Thank you for your contribution and for participating on this special day 🥇. Overall, the code looks good to me and works correctly. I left a couple of tiny suggestions, but nothing huge.
While testing, however, I noticed that depending on the product names involved, the message can look a bit confusing, especially when variable products are added to the mix, as those can output commas (,
) as part of their names. For example:
The use of colons to indicate IDs might also be confusing given colons are also used before the product names list.
I was wondering if, for simplicity's sake, we should only list the product IDs for which we're holding stock (no names) or just a generic message with no specific IDs or names.
@vedanshujain: as the author of the original issue, did you have any specific formatting in mind? Do you think a message with just product IDs or a generic message work here?
If not, we would need to be a bit more clever on how to separate the product names to prevent any confusion (or notes that are too large).
@Abdalsalaam: code-wise this looks good to me (with the few exceptions mentioned in the review), but I'd like to hear from @vedanshujain first. If you have some ideas on how to change the presentation to make things clearer, by all means please share those too!
Once again, thanks for all your hard work on this.
Thank you @jorgeatorres ! I did the changes, for the note I think its good to have the product name in the note, I suggest to use / instead of commas to separate products, also we can use X for the quantity, it will be like this : Stock held for 10080 minutes for payment: Product (ID: #6293), Color: Green, Attribute: Red x 1 / Import Product - Variable - Yellow (ID: #5953) x 1" what do you think? |
Hi @Abdalsalaam! I particularly like the last idea, with the dash/bullet list. I think that looks much clearer. I also think we should limit the number of products that are listed. If the order contains tons of items, the note could end up being too large. Maybe we should only apply it to the first 5 products and if there are more, we just add something like "- ... and N more items." That's obviously less useful, but also less noisy. What are your thoughts on that? Definitely not a blocker, but worth discussing now that we're working on this. Looking forward to hearing back from you! |
I like your Idea, i think we should it it now yes. let me an edit for it. |
What do you think? |
Hi @Abdalsalaam, Thanks for making the changes. I think it looks really great. I'll go through a re-review as soon as possible! |
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.
Thanks again for your contribution and all the work you put into this 🥇. I pushed a couple of commits with some small adjustments to the code (we were missing a couple of translatable strings, etc.) but overall, this works and looks great.
As such, I'm approving now. It's been a pleasure working with you in this PR and hope we'll be seeing more contributions from you, not only on Contributor Day 😸
Thanks!
Steps to test
|
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #29132 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions: