-
Notifications
You must be signed in to change notification settings - Fork 9.4k
[TASK] Fixed undefined shipping method name in checkout summary #22365
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
Conversation
|
Hi @basvanpoppel. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
|
Bas van Poppel seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
orlangur
left a comment
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.
@basvanpoppel it is not a good idea to duplicate this code, please refactor.
Jensderond
left a comment
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.
Using a dot notation for the objects makes the code easier to read.
See: https://eslint.org/docs/2.0.0/rules/dot-notation
Thank you for the reply, i will make a new commit later this week. I'm a bit uncertain about 'refactoring, but I have an idea what the best approach should be. |
|
@basvanpoppel basically we need one implementation used in two places: place with the old fix and the new one you performed. |
|
@basvanpoppel , I am closing this PR now due to inactivity. |
|
Hi @basvanpoppel, thank you for your contribution! |
Description (*)
In checkout on the last step, the shipping method is displayed as 'undefined' when no name is set. The code in this PR makes sure the name is only displayed when it's defined.
There is a merged PR related to this, which didn't solve the issue completely (#17697)
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)