fix: improve namespace and variables naming#65
Conversation
|
Thanks for the pull request, @MaferMazu! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
|
I would like to add a few changes to the CSS variable naming (I want to ensure we are not editing some general CSS styles and colliding with other plugins), but if you have early feedback, please let me know. |
felipemontoya
left a comment
There was a problem hiding this comment.
@MaferMazu I looked at the changes and it looks reasonable.
In the review it looks like you didn't change the names of anything that becomes a post_metadata, right?
If I'm correct then we should be able to merge and the changes would be backwards compatible with enrollment_requests that were already sent with the old var names. We can verify this by uploading this code in the staging and testing the admin page for the old requests. If everything works we could go ahead and merge.
feanil
left a comment
There was a problem hiding this comment.
Generally looks good to me but I had one question about a few variables that were not renamed.
|
Yes, @felipemontoya, I am not touching the post_metadata variables. I have tested the plugin locally and in the Stage with the changes. You can double-check in the Stage with these credentials: https://share.1password.com/s#mjFE_KtE55oS9OrVUCMw0aZw8r4BvF-6ONKeFxJ2BuI. And I renamed the variables for consistency, as @feanil suggested. About the CSS variables I wanted to check, I checked, and they don't change the behavior of other plugins. I think we can go with this and send the ZIP file. |
felipemontoya
left a comment
There was a problem hiding this comment.
Thanks for the updates and fixes @MaferMazu.
I think we can merge this and present the zip for publication
|
@MaferMazu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR:
ApptoOpenedXCommerce.openedxas the prefix, e.g.,enrollment_course_idtoopenedx_enrollment_course_id.Testing instructions
For testing purposes, you can:
Note:
Additional information
This PR is for avoiding WordPress's common mistakes, in particular, the following:
Note
Before merging this PR, I want to ensure we are not editing some general CSS styles and colliding with other plugins.
Checklist for Merge