Skip to content
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

fix(ia): core update nag margins #3531

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Conversation

miguelpeixe
Copy link
Member

@miguelpeixe miguelpeixe commented Nov 7, 2024

All Submissions:

Changes proposed in this Pull Request:

1205919985867982-as-1208354721118905/f

Fixes the core update nag margins when navigating Newspack wizards for both full wizard pages and header-only:

Before After
Header-only image image
Wizard page image image

How to test the changes in this Pull Request:

  1. Checkout this branch, navigate to the wizard pages and admin-header-only pages
  2. Confirm the margins are correct, as in the images above

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@miguelpeixe miguelpeixe self-assigned this Nov 7, 2024
@miguelpeixe miguelpeixe requested a review from a team as a code owner November 7, 2024 13:51
@ronchambers ronchambers merged commit d65ce26 into epic/ia Nov 8, 2024
7 checks passed
@ronchambers
Copy link
Collaborator

ronchambers commented Nov 8, 2024

Sorry @miguelpeixe - I accidentally merged this directly into epic/ia from my local. I meant to push up a recommendation for the CSS, but I ended up merging this PR along with merging my recommendation too! Sorry. Anyway, I hope you like what I have to say below because it's already all merged 😉 If you don't like the following then I can roll the code back.

In my review (and merge) I was able to get the CSS down to just one block. Please see:

55ee135

This one block of CSS works for both full-page react and admin-only too. Additionally I updated it to work not just on "update-nag" but for all notices. I adjusted the margin down to 20px instead of 25px to cut down on some of the whitespace when multiple notices are present. And finally the CSS is only applied to "above header" notices, not in "wrap" (content) notices. It's been thoroughly tested.

I'll attach some screenshots below.

@ronchambers
Copy link
Collaborator

Full React Page:

image

Admin Header Page:

image

@ronchambers
Copy link
Collaborator

I'm adding a "mental note" here because there are some wizard pages that don't show any admin notices at all. It took me a while to figure out why that was while I was doing testing.

Some examples are:

  • /wp-admin/admin.php?page=newspack-settings#/
  • /wp-admin/admin.php?page=advertising-display-ads#/

These pages don't show any admin notices due to lines:

add_action( 'admin_notices', [ $this, 'remove_notifications' ], -9999 );
add_action( 'network_admin_notices', [ $this, 'remove_notifications' ], -9999 );
add_action( 'all_admin_notices', [ $this, 'remove_notifications' ], -9999 );

And function:

public function remove_notifications() {

Just an FYI on why there are no notices on those pages.

@miguelpeixe miguelpeixe deleted the fix/ia-update-nag-margin branch November 13, 2024 13:29
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