-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Steps to add custom field in payment #8940
Steps to add custom field in payment #8940
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.
Before I review fully, there were two points that were on the old PR that were not addressed.
POINT 1
Not sure if Steps 1 & 2 need to be this detailed. Just say create Learning/CustomField and register it. Perhaps a link to : https://devdocs.magento.com/videos/fundamentals/create-a-new-module/
POINT 2
This is now a deprecated method. This should be done with a Patch / Declarative Schema : https://devdocs.magento.com/guides/v2.3/extension-dev-guide/declarative-schema/
Whilst not complusory for 2.3 it is now the accepted way and would also apply to the v2.4 of this guide and futureproof the content.
Also database changes should not be done using InstallData, it should be done by a InstallSchema - you have mixed the two.
@dobooth - can we get a second opinion on this? As a 2.4v is needed and 2.3 support it, should this not be done using the declarative schema method?
Please can these be addressed before a detailed check is done?
@BarnyShergold I will fix the Point 1 and update the PR. |
Thanks for the discussion, gents. We should use the declarative schema. We certainly don't want to add new content that we know will be out of date. Let's do it properly. We can help if needed. |
@BarnyShergold @dobooth Updated the doc with the declarative schema approach. Please review 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.
@ajithkumar-maragathavel This is a great start. It'll probably take 3 or 4 rounds of comments to finalize this PR.
@keharper Thanks for your time for the detailed review. I have updated the requested changes. Let me know if everything fine. |
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.
Apologies for taking this long to respond to your previous round of changes.
Forgot to tag @ajithkumar-maragathavel |
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.
@ajithkumar-maragathavel There are still quite a few comments from my previous review that have not been addressed. I'll only comment on the new material here.
@ajithkumar-maragathavel How's this one coming along? |
hey @dobooth yep it's coming good so far. Had couple of rounds of review for this PR. |
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 your patience, @ajithkumar-maragathavel. From my perspective, this PR is ready to be merged. @BarnyShergold, any further comments?
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.
This is an awesome piece of work - looks super clear and easy to follow.
Gold ⭐ !
Great teamwork here. Thanks for all the effort! |
@magento import pr to magento-devdocs/devdocs |
@dobooth the pull request successfully imported. |
Hi @ajithkumar-maragathavel, thank you for your contribution! |
Purpose of this pull request
This pull request (PR) adds a new page that explains adding a custom input field on the payment section on the checkout payment step.
Affected DevDocs pages
Links to Magento source code
Related Pull Requests
Fixes #8927
whatsnew
Added the topic Add a custom field for an offline payment method to the Customize Checkout tutorial.