Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.58k stars 799 forks source link

Odyssey stats styling seems to be overriding all admin notices in /wp-admin/ #31375

Open Nic-Sevic opened 1 year ago

Nic-Sevic commented 1 year ago

Impacted plugin

None / Other

Quick summary

The styling of admin notices seems to be getting overwritten by the styling here: dde63222

Notices initially load correctly and then the odyssey stats styling is applied. This is happening only on site.com/wp-admin and not the /wp-admin/something pages seems to be happening with all admin notices

Steps to reproduce

  1. On a WoA site (pro/business/ecommerce) force an admin notice to show on /wp-admin/ (An easy to force notice is to activate the anyone can register setting in /wp-admin/options-general.php)
  2. Check how the notice displays on a different wp-admin page (ex: wp-admin/options-general.php)
  3. compare to styling on /wp-admin/

A clear and concise description of what you expected to happen.

admin notice styling should be the same on all wp-admin pages, not overwritten

Expect it to look something like this (left sidebar color would vary) 2023-06-14 at 14 57

What actually happened

the notice is instead styled like 2023-06-14 at 14 58

Impact

All

Available workarounds?

No but the platform is still usable

Platform (Simple and/or Atomic)

Atomic

Logs or notes

No response

kangzj commented 1 year ago

This is fixed in #78221 - Thanks @dognose24! and shipped with D113773-code.

dognose24 commented 1 year ago

@kangzj Thanks for shipping with the Odyssey Stats build! 🚢

Nic-Sevic commented 1 year ago

Hey @dognose24 It looks like this partially fixed things but the color of the notice on the lefthand side is still getting overwritten. Currently it seems that regardless of the color it's supposed to show up as it's showing as grey

dognose24 commented 1 year ago

Hi, @Nic-Sevic! Could you give me more clues (e.g., screenshots) to demonstrate the remaining styling issue? The notice styling on my Atomic site seems reasonable.

截圖 2023-06-27 上午11 39 04
Nic-Sevic commented 1 year ago

Hi @dognose24, when I check my sites I'm still seeing the notice like 2023-06-28 at 10 11

when it should be like 2023-06-28 at 10 11

Are you maybe looking on a self-hosted site rather than a WoA site? Or maybe something in your dev site isn't fully matching?

dognose24 commented 1 year ago
截圖 2023-06-29 上午12 38 35

I confirmed the issue was caused by the .notice__icon-wrapper styles of wpcomsh overridden by Calypso Notice component styles imported via Odyssey Stats.

Since the notice component would be used spreadly inside Calypso, changing the component styles to CSS modules might break specific existing styling. I would prefer to increase the specificity of the wpcomsh notice from the source, and the changes look reasonable as well.

dd32 commented 1 year ago

I've run into some styling issues coming from the odyssey-stats styles too, this time that the wp-calypso styles for .notice:not(.wpcomsh-notice) is too broad, applying to wp-admin core notices too.

For example, it's visible in the Events widget on self-hosted sites using Jetpack Stats:

Screenshot 2023-07-06 at 12 58 16 pm

Ideally, the component imported from Calypso which styles .notice probably needs to be scoped to body.calypso .notice or similar. https://github.com/Automattic/wp-calypso/blob/trunk/apps/odyssey-stats/src/styles/wp-admin.scss

dognose24 commented 1 year ago

I've run into some styling issues coming from the odyssey-stats styles too, this time that the wp-calypso styles for .notice:not(.wpcomsh-notice) is too broad, applying to wp-admin core notices too.

Yes, we are conscious that specific resolutions for reported conflict styles might still cause similar issues in the future.

Ideally, the component imported from Calypso which styles .notice probably needs to be scoped to body.calypso .notice or similar.

Scoping the Calypso Notice component styles could be a fundamental solution. However, I am still determining the impact of refactoring the Calypso Notice component usage with customized overriding styles, which could also cause potential issues.

github-actions[bot] commented 8 months ago

This issue has been marked as stale. This happened because:

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.