Skip to content

Conversation

@vk17-starlord
Copy link

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

@github-actions
Copy link

github-actions bot commented Oct 3, 2024

Hi @vk17-starlord! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

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 WordPress Project

@github-actions
Copy link

github-actions bot commented Oct 3, 2024

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props vineet2003, joedolson.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions
Copy link

github-actions bot commented Oct 3, 2024

Test using WordPress Playground

The 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

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Copy link
Contributor

@joedolson joedolson left a 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.

// Support for multiple message formats
var message = data.message;
if (data.successes || data.errors) {
message = 'Success: ' + data.successes + ' operations completed. Errors: ' + data.errors;
Copy link
Contributor

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.

Copy link
Author

@vk17-starlord vk17-starlord Oct 7, 2024

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>';

Copy link
Contributor

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.

@vk17-starlord
Copy link
Author

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:

  1. type: For notice types (error, success, warning, info).
  2. dismissible: To determine if the notice can be dismissed.
  3. id: A unique identifier for the notice.
  4. additional_classes: An array of extra CSS classes.
  5. attributes: Extra HTML attributes for the notice element.
  6. paragraph_wrap: To determine if the message should be wrapped in a p tag.

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 .

@vk17-starlord
Copy link
Author

	var addAdminNotice = function( data ) {
		var $notice = $( data.selector ),
			$headerEnd = $( '.wp-header-end' ),
			type,
			dismissible,
			additionalClasses = '',
			attributes = '',
			paragraphWrap = ( data.paragraph_wrap !== undefined ) ? data.paragraph_wrap : true,
			$adminNotice;
	
		// Defaults for the arguments.
		type = data.type ? data.type : 'info';
		dismissible = ( data.dismissible && data.dismissible === true ) ? ' is-dismissible' : '';
		var message = data.message ? data.message : '';
	
		// Handle additional classes if any are provided.
		if ( data.additional_classes && Array.isArray( data.additional_classes ) ) {
			additionalClasses = ' ' + data.additional_classes.join( ' ' );
		}
	
		// Handle additional attributes if any are provided.
		if ( data.attributes && typeof data.attributes === 'object' ) {
			Object.keys( data.attributes ).forEach(function( key ) {
				attributes += ' ' + key + '="' + data.attributes[key] + '"';
			});
		}
	
		// Wrap message in <p> if paragraph_wrap is true.
		if ( paragraphWrap ) {
			message = '<p>' + message + '</p>';
		}
	
		// Build the admin notice element.
		$adminNotice = '<div id="' + data.id + '" class="notice notice-' + type + dismissible + additionalClasses + '"' + attributes + '>' + message + '</div>';
	
		// Check if this admin notice already exists.
		if ( ! $notice.length ) {
			$notice = $( '#' + data.id );
		}
	
		// Either replace an existing notice or insert a new one.
		if ( $notice.length ) {
			$notice.replaceWith( $adminNotice );
		} else if ( $headerEnd.length ) {
			$headerEnd.after( $adminNotice );
		} else {
			if ( 'customize' === pagenow ) {
				$( '.customize-themes-notifications' ).append( $adminNotice );
			} else {
				$( '.wrap' ).find( '> h1' ).after( $adminNotice );
			}
		}
	
		// Trigger a global event after adding the notice.
		$(document).trigger( 'wp-notice-added' );
	};

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.

@joedolson
Copy link
Contributor

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.

@vk17-starlord
Copy link
Author

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 😄 !!!

@vk17-starlord
Copy link
Author

@joedolson - please check this function i have added in latest commit

@vk17-starlord
Copy link
Author

@joedolson anything left from my side to be done for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants