-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Added : Refactor admin notice handling to eliminate duplication #7485
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
base: trunk
Are you sure you want to change the base?
Added : Refactor admin notice handling to eliminate duplication #7485
Conversation
|
Hi @vk17-starlord! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
joedolson
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.
This is mostly adequate for fitting this one use case, but for abstracting it out for broader support it's going to need a bit more. Ideally, it should be a reasonably close match for how the PHP equivalent function works, so that the API expectations match.
Take a look at https://developer.wordpress.org/reference/functions/wp_admin_notice/ and the arguments it accepts. They don't need to have the same structure, but it should support a similar variety of options.
You may want to look through the various admin notices that are currently generated in JS to see what specifically needs to be supported.
src/js/_enqueues/admin/common.js
Outdated
| // Support for multiple message formats | ||
| var message = data.message; | ||
| if (data.successes || data.errors) { | ||
| message = 'Success: ' + data.successes + ' operations completed. Errors: ' + data.errors; |
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 looks like it was here for testing; the message probably doesn't need to be emended.
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.
Is this what you mean by "the message probably doesn't need to be emended. "
var message = data.message;
$adminNotice = '<div id="' + data.id + '" class="notice notice-' + type + dismissible + '"><p>' + message + '</p></div>';
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 mean that the line message = 'Success: ' + data.successes + ' operations completed. Errors: ' + data.errors; doesn't need to be there.
|
Do we need to rewrite this function in a way that it needs to merge the passed data into a default configuration, similar to how the PHP equivalent works ? Handle custom classes and additional attributes flexibly. What i think we can do is The addAdminNotice() function should accept parameters like:
I will do this changes and will update the PR @joedolson but just let me know if this is what we actually want or its in the wrong direction . |
Have a look at this @joedolson we can do this to make our existing JS function handle additional requirements similar to PHP admin notice arguments. |
|
Yes, that's what I had in mind. We need to anticipate possible usages, so that if extenders start to use this, it's flexible enough for usage. |
Anything else to be done or is this patch ready to be merged .... it will be my first contribution 😄 !!! |
|
@joedolson - please check this function i have added in latest commit |
|
@joedolson anything left from my side to be done for this PR? |
This PR refactors the handling of admin notices in WordPress to eliminate code duplication and enhance standardization across the platform. The following key changes have been implemented:
Centralization of Admin Notice Functionality: The addAdminNotice() function in common.js has been refactored to support all arguments previously handled by wp.updates.addAdminNotice(). This allows for a unified approach to displaying admin notices with enhanced flexibility for various use cases (e.g., bulk editing, updates).
Backward Compatibility: The wp.updates.addAdminNotice() function in updates.js has been modified to simply delegate calls to the newly refactored addAdminNotice() function. This change ensures that existing usages of wp.updates.addAdminNotice() continue to function without issues, maintaining backward compatibility.
Improved Message Formatting: The new implementation supports multiple message formats, including success and error messages, thereby providing users with clearer feedback when actions are performed in the admin area.
Trac Ticket
Trac ticket: #62139