-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Remove "ready for" info from shipping method description area at checkout #12395 (solved) #12443
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.
Welcome and thank you for contributing on this issue!
I think it needs to change before it's ready, can you please see my comments and make the requested changes?
Additionally, you don't need to merge master in to keep the branch up to date. This causes additional unnecessary commits in the history.
@@ -93,7 +93,7 @@ | |||
%em.fees= payment_or_shipping_price(shipping_method, @order) | |||
- display_ship_address = display_ship_address || (ship_method_is_selected && shipping_method.require_ship_address) | |||
%div.checkout-input{"data-shippingmethod-target": "shippingMethodDescription", "data-shippingmethodid": shipping_method.id , style: "display: #{ship_method_is_selected ? 'block' : 'none'}" } | |||
#distributor_address.panel | |||
-# #distributor_address.panel | |||
- if shipping_method.description.present? |
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.
We prefer not to keep commented-out code, so you can go ahead and delete the code as per the requirements. If it's required in the future, it shouldn't be hard put back (we can check the git history). Because the codebase is so big, it would be too hard to manage if we kept unused code.
However, from the picture it looks like we want to keep the shipping method description, and only remove the "ready for" section. I think that means removing lines 99-103.
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.
Will change the code in order to remove only the "ready for" section as noted by dacook.
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.
Your assumption is correct, i have to delete lines 99-103.
There are some failing specs now. Looks like CI is not happy with the indentation :) |
Updated the local branch in order to see better why the checks are failing. |
#distributor_address.panel | ||
#distributor_address.panel |
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 think you can revert this change. Wouldn't that fix the indentation failures you get?
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.
yes, thanks
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.
good eye
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.
Great! Thank you.
This provides the solution wanted and the best way to do it.
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 for adjusting that, looks good! I've updated the commit history to show just the one overall change.
We will add to the testing queue, and after it's manually tested it can be merged!
Hey @MrBowmanXD , welcome!! I've tested your PR, and looking at the acceptance criteria on #12395, I could verify that, on the -> before this PR -> after this PR So, we can see that the Also, I've verified this is appearing on the other pages/places: -> at the top of the checkout pages (verified for all three steps) -> order confirmation page -> order confirmation emails -> on the dropdown of the shop Looking good! |
What? Why?
sometimes, some shipping method are not available every day. It can raise a conflict with the content the shop manager puts in the "ready for" field. Therefore, we should remove the display of the field in the shipping method description. (according to the creator of the issue)
This change simply comments the code related to the "ready for" info from shipping method description area at checkout/details. Instead of running the code it simply does not execute because it is commented (line 96 until 103 not working).
What should we test?
(these steps were done manually to check)
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.