Combine/globalize the CSS/JS between Site Health & Privacy Settings

[xkon]

I'm opening this early on and marking for 5.8 after some discussions over slack with @Clorith.

Since the new Privacy Settings design was based on the existing Site Health design, it is using similar features (tabs menu, accordions etc). The CSS & JS used are a "copy" of the existing Site Health code with some minor alterations and renaming. It was done this way to avoid confusion and keeping things a bit more simple for the time being.

The idea of the discussion was to see if we can unify and make at least the common code available in general.

This will help with avoiding issues in future updates as we might make changes on one UI but forget to "sync" the changes to the other, as well as possibly help other screens that want to use similar styling or functionality.

[helen]

This needs an owner (can be self-assigned) and at least a preliminary patch in the next week or else this likely needs to be moved to a later release, as we will enter bugs-only the week after that. I imagine this needs some thought about whether to actually make the selectors generic and meant for reuse, or "just" rely on the existing ones and combine for now.

For declaration blocks in site-health.css that are duplicates of those for Privacy Settings in edit.css, add the selectors from site-health.css to the ruleset in edit.css to combine these duplicate styles.

Trac ticket: https://core.trac.wordpress.org/ticket/52429

[notlaura]

We discussed this ticket in the #core-css meeting last week and decided combining the selectors for now makes the most sense, and they can be generalized at a later date. There has been conversation from design to use standardize the header style across core, but according to @melchoyce this is not expected to be a priority until after full-site editing at the earliest.

The patch here adds selectors that have duplicate styling from site-health.css to edit.css, and removes the duplicates from site-health.css.

[Clorith]

I had a quick look at the proposed patch, I'm a but confused as to why we're moving everything into a catch-all file of edit.css here? If I'm not mistaken, the privacy things were only meant to be there temporarily because it was introduced in a minor release where no new files should be added, and it was meant to have its own stylesheet after this?

Is edit.css always enqueued on every page, I see ​some plugins are enqueueing `site-health` styles them selves, so we want to avoid styles breaking for them as well if possible.

The modifications also appear to have negated some styles used by the Site Health dashboard widget, which were defined, and built upon, styles from the site-health stylesheet.

[ryelle]

If we create a new file just for the shared CSS, then add it as a dependency of site-health & edit, that should keep the expected styles for plugins in both places, right? I don't know if it's worth creating another new stylesheet just for the privacy styles, once you remove the shared styles there's <100 lines of CSS left.

The modifications also appear to have negated some styles used by the Site Health dashboard widget, which were defined, and built upon, styles from the site-health stylesheet.

It looks like this is from "Use flex for alignment in .health-check-title-section vs. only text-align" - the flex is messing up the layout in the widget. I think it would make more sense to use different classes here, but I haven't thoroughly investigated it.

[prbot]

​ryelle commented on ​PR #1266:

I took a few screenshots of Site Health & Privacy (both tabs) at 3 sizes (960, 650, 400) & ran them through a diff script - mostly everything is similar, but it did highlight some differences.

✅ First in Privacy, as you mentioned, both pages are offset by 8px — I think this is a good change, it evens out the header visually.

✅ Privacy policy (same offset non-issue)

⚠️ Site Health Status: The badge has a margin now when it should not, moving it over slightly

✅ Site Health Info: no change

⚠️ Dashboard: as mentioned on the ticket, the flex style is breaking the widget a bit

[prbot]

​laras126 commented on ​PR #1266:

@ryelle thank you! What tool are you using to generate these diffs? Is that related to #209, or something you are doing locally?

I believe I have resolved the two problematic issues from your screenshots, and added a new selector for .health-check-widget-title-section.

[notlaura]

@Clorith Thank you for the notes – I was not familiar with this history, so this is helpful to know. I chose to consolidate the styles in edit.css because it looked to be enqueued on each admin screen – I am not 100% sure of this though, and need to validate it.

My goal with this approach was to make it possible to edit CSS for both areas in a single location for the 5.8 milestone. I feel that moving the styles to a different stylesheet and generalizing the selector names would be better suited for a future release when there is more clarity about how/where these styles will be used. Then, we can intentionally choose generic selector names and introduce the overhead of a new stylesheet once there is a plan to utilize it for more than these couple of components.

Again though, I am new to the history of these features and this CSS, so please let me know if my thinking is off-base here!

[Clorith]

No objections here so long as we don't negatively affect anything :)

I'm not sure on the history, of why the privacy component just copied styles with new class names, instead of re-using the existing ones (if the desire was to generalize them after), but I see there's some details in #49264

I think your reasoning for using edit.css makes sense then, we just need to make sure the styles apply where expected then 👍

[ryelle]

I think the only case where this would remove styles from plugins is if they're using site-health on the frontend, or have intentionally un-enqueued wp-admin. I think the latter is unlikely. Do you think anyone's using site-health on the frontend? I looked through a few of the results from the search and they seem to be wp-admin pages, so this makes no change.

The PR is looking good, the issues I flagged have been fixed ✅

What tool are you using to generate these diffs? Is that related to #49606, or something you are doing locally?

It's local, I have a framework for ​grabbing screenshots with puppeteer and I've been writing little scripts for each ticket I review, then using the npm run diff command (in that project) to run ​blink-diff over them. (I keep meaning to write a blog post about this 😅 )

[ryelle]

I think this should get in since the PR looks good, and any plugin compat issues can be looked into during beta. Will commit it shortly :)

[ryelle]

In 51025:

Site Health, Privacy: Combine shared CSS for Site Health & Privacy Settings

The styles for the Privacy settings page were based on the Site Health section. These were duplicated into edit.css in #49264. This change merges the selectors from Site Health into the Privacy section, to reduce that duplicate code.

Props xkon, notlaura, clorith.
See #52429.

[prbot]

​ryelle commented on ​PR #1266:

Committed in r51025 🎉

[ryelle]

Leaving this ticket open, but moving to 5.9. Now that the CSS has been combined, we can create some shared selectors for the relevant styles. We should also look into the shared javascript functionality — in a quick scan I just saw the accordion toggles, but there might be more.

[pbearne]

I will create a generalized version

this is the generalized version of the css

Trac ticket: https://core.trac.wordpress.org/ticket/52429

[pbearne]

I have created the generalized version of the CSS
I used "admin-page" as the new tag

[ryelle]

Hi @pbearne, thanks for the PR!

I'm not sure about admin-page-* as the prefix. Maybe wp-core-ui-*? There's already a body class that uses that term, though I'm not sure of its history. IMO, this prefix should identify that this is part of a WordPress core "component library" of sorts.

For backwards compatibility, I think the old class names should stay in the HTML & CSS, and you can add the new class names to the components, like so:

.privacy-settings-accordion-heading,
.health-check-accordion-heading,
.wp-core-ui-accordion-heading {
	...

<h4 class="wp-core-ui-accordion-heading privacy-settings-accordion-heading">

That way, if any plugins are using the existing classes to inherit these styles, they'll still work, while any new pages will be written using the shared classes.

[pbearne]

I am happy to make the name change.

I can see the point in keeping the CSS, but I would like to see if we can get a scan of the plugin director for uses

But I don't believe we should keep the old class in the HTML as we control that

[sabernhardt]

With WP 5.9 feature freeze tomorrow, I'm moving this ticket to Future release.