[Site Admin]: introduce `<SiteIcon />` component #99540

retrofox · 2025-02-10T12:25:07Z

~~# ❗ Branched off from #99492~~

This PR introduces the component, which is heavily inspired by the component defined in Core.

Similar to how most components in the Site Editor implementation (@wordpress/edit-site package) work, this component primarily retrieves and assigns values through stores rather than exposing specific props.

For example, in this particular case, the component does not provide a prop for setting the site icon image. In fact, the only exposed prop is className. The site icon image URL is determined through the store.

Storybook

Due to this approach and to enable proper visual testing of this and future components, this PR also introduces a simple yet efficient mechanism (at least for this component) to mock the store from which the SiteIcon component retrieves the URL for rendering the icon.

A mocked store is created with three different sites, each having a distinct icon. This setup allows switching between test sites (see testing instructions below).

Future PRs can further improve this mocking system.

Close #99531
Close #99479

Proposed Changes

[Site Admin]: introduce component

Why are these changes being made?

Testing Instructions

With storybook:

Update dependencies:

yarn install

Run the Storybook instance for this package:

yarn workspace @automattic/site-admin run storybook

Verify that the icon is correctly displayed in the Storybook story.

Use the global control to test different site types.

Screen.Recording.2025-02-11.at.12.18.59.PM.mov

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
- For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2025-02-10T12:25:29Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-132910

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-132910&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-132910&env=a8c-for-agencies

matticbot · 2025-02-10T12:35:39Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

mirka

I'm not 100% sure on where we are with the "staying as close as we can to the Core implementations" idea. This code pretty much looks like a fork of the Core implementation, and yet I see many issues which (if we were to address) cannot be addressed by minor tweaks that are easily diff-able with Core. Even if we're not thinking about merging things back to Core, there will still be difficulties merging in Core updates back into our fork.

Reading the principles in #99473 again after seeing this code, I wonder how we're going to balance "optimize for our needs" and "maintaining consistency with Core". Maybe one approach is to not care too much about maintaining consistency?

(My comments are non-blocking, I understand we're in the quick iteration phase.)

packages/site-admin/package.json

packages/site-admin/src/components/site-icon/index.tsx

mirka · 2025-02-12T08:46:55Z

packages/site-admin/src/components/site-icon/index.tsx

+	const { isRequestingSite, siteIconUrl } = useSelect( ( select ) => {
+		const { getEntityRecord } = select( coreDataStore );
+		const siteData = getEntityRecord( 'root', '__unstableBase', undefined );
+
+		const site_icon_url = getEntityRecord< {
+			site_icon_url: string;
+		} >( 'root', '__unstableBase' )?.site_icon_url;
+
+		return {
+			isRequestingSite: ! siteData,
+			siteIconUrl: site_icon_url,
+		};
+	}, [] );


I'm generally skeptical about UI components that are internally coupled to a store — is this going to be the case for all the other UI components as well? This one is pretty simple but I'm sure there are more complex ones. I feel like it would be easier to test and maintain if there was one level of separation. (One base component with props, another wrapped component with the store subscription.)

Also related, I'm seeing that the SiteIcon component is exported publicly. This implies to me that the component is going to be reused by consumers in custom UIs. That doesn't quite click with me when I see the tight store coupling, and the pretty specific CSS conditionals with the external selectors in the stylesheet.

If it doesn't make sense to publicly export the component, then it may be overkill to have to maintain a Storybook story for it. A unit test may be better suited to ensure that the store connection is working as expected.

Core seems to have a clear intent to make these components store-dependent, not just this one but others I’ve worked with.

Decoupling them for flexibility isn’t a bad idea, but I believe the goal behind keeping them tightly coupled is to prevent improper usage.

This does make testing more restrictive—especially in Storybook—but it’s not a blocker, just a headache. In the current Storybook setup, we mock the store to showcase how the component adapts to different data states.

Also related, I'm seeing that the SiteIcon component is exported publicly.

That's a good point. I'm going to remove it.

A unit test may be better suited to ensure that the store connection is working as expected.

Something that we could do in a follow-up?

I guess the use case in this PR doesn't make a great example of why the store coupling is necessary, but if you could provide some examples of what we're planning to do, that could be helpful.

The only good example I can offer is the current implementation of the Site Editor in core.
The core version of this component and others pull data from the store.

I will research/ask those who implemented that at some point what the motivation is.

If we don't have a good use case for any feature, let's consider untangling / removing / cleaning up. Now that we're creating new components it only makes sense to start fresh and clean.

Sorry, I think we’re not quite on the same page regarding the development of this package. The initial idea is simple: to base our implementation on core’s approach to ensure consistency. This allows us to build on what I consider solid foundations.

If we later find that something isn’t quite right, we can always adapt or even remove it, but I don’t think this is the right stage to do that.

In this particular case, I don’t have a concrete answer as to why we are pulling properties from the store instead of passing them as props to the SiteIcon component. However, this implementation exists in core, which suggests it has been thoroughly researched and developed with a clear purpose. I’m not advocating for blindly following core, but at this point, I’d rather rely on an established solution than introduce changes without a strong justification.

Additionally, this PR already includes significant research and testing. We have identified the minimal and necessary resources needed for the new WooCommerce Analytics admin interface, a standalone plugin we are developing. While I can’t guarantee that this implementation will scale perfectly with future needs, the results so far have been more than satisfactory.

That said, I’d like to flip the question: why do you think it’s better to propagate these data points via props instead of retrieving them from the store? I believe this is more about understanding the engineering approach used in this implementation as a whole.

From my perspective, it makes much more sense for the component to adapt to the context in which it is instantiated automatically. In this case, the component is called SiteIcon, which suggests that it represents the site’s icon. Why should we manually define the icon’s URL when we know it’s not an arbitrary value but something that exclusively depends on the global configuration?

If what is needed is a generic component to render an image or an icon, then a component designed specifically for that purpose should be used instead.

I hope this clarifies my perspective.

packages/site-admin/src/components/site-icon/stories/index.stories.tsx

mirka · 2025-02-12T08:56:50Z

packages/site-admin/src/components/site-icon/style.scss

+	}
+}
+
+.site-admin-editor__view-mode-toggle button:focus {


It's not great that a .site-admin-editor__view-mode-toggle selector is living in this file. .site-admin-layout.is-full-canvas is also a bit worrisome. Might be something to refactor eventually.

Makes sense. Let's add a note about it, and refactoring when introducing the other components.

packages/site-admin/src/components/site-icon/stories/style.stories.scss

packages/site-admin/.storybook/preview.tsx

retrofox · 2025-02-12T10:51:57Z

I'm not 100% sure on where we are with the "staying as close as we can to the Core implementations" idea. This code pretty much looks like a fork of the Core implementation, and yet I see many issues which (if we were to address) cannot be addressed by minor tweaks that are easily diff-able with Core. Even if we're not thinking about merging things back to Core, there will still be difficulties merging in Core updates back into our fork.

Reading the principles in #99473 again after seeing this code, I wonder how we're going to balance "optimize for our needs" and "maintaining consistency with Core". Maybe one approach is to not care too much about maintaining consistency?

(My comments are non-blocking, I understand we're in the quick iteration phase.)

Thanks for your review, Lena. It’s constructive and touches on the most critical aspects of this implementation.

Regarding how closely we should align with Core in the long run, the key takeaway (based on input from other developers involved) is to prioritize compatibility regarding UI/UX, without a strict requirement to follow the same technical implementations.

The reason this feels almost like a fork (at least from my perspective) is that I’ve used Core as a development guide. That said, I believe the best strategy is to define clear phases. In this initial stage, the priority is to deliver a functional system within a reasonable timeframe. We’re struggling to gain the right momentum right now, and I think we should focus on reducing friction.

Later on, with more time, we can refine the implementation—even if that means deviating from the ideal Core-aligned approach. For example, decoupling components from the store is worth considering, but I don’t think it should be a priority at this stage.

Ultimately, the goal isn’t to maintain strict compatibility with Core but to build an independent implementation based on it, evolving it according to our needs. And if it makes sense, we could even consider upstreaming this implementation to Core when it gets an option.

mirka · 2025-02-12T12:30:00Z

The reason this feels almost like a fork (at least from my perspective) is that I’ve used Core as a development guide. That said, I believe the best strategy is to define clear phases. In this initial stage, the priority is to deliver a functional system within a reasonable timeframe. We’re struggling to gain the right momentum right now, and I think we should focus on reducing friction.

Understood 👍 I can calibrate my reviews for that.

mirka

🚀

Co-authored-by: Lena Morita <[email protected]>

matticbot · 2025-02-12T13:57:05Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/site-admin-introduce-site-icon-core-copy on your sandbox.

tyxla · 2025-02-12T14:34:46Z

packages/site-admin/.storybook/mock-registry/index.ts

+/**
+ * Creates a global mock registry for @wordpress/data
+ */
+export const createMockRegistry = ( stores = {} ) => {


Is there a good reason to have a default stores value? I don't think so. Some improved types could help ensure we pass a stores.

Suggested change

export const createMockRegistry = ( stores = {} ) => {

export const createMockRegistry = ( stores ) => {

Going to address it in a follow-up.

tyxla · 2025-02-12T14:36:17Z

packages/site-admin/.storybook/mock-registry/stores.ts

+			name: 'WooCommerce Analytics',
+			description:
+				'WooCommerce Analytics is a powerful tool that helps you understand how your store is performing and how you can improve your store’s performance.',
+			'root/__unstableBase': { site_icon_url: './woo-logo.png' },


Can we reuse existing logos from the repo instead of adding new copies?

We can. I just wanted to isolate the story resources in its folder. I can update you if you think it would be better. 👍

tyxla · 2025-02-12T14:37:41Z

packages/site-admin/src/components/site-icon/index.tsx

+	const { isRequestingSite, siteIconUrl } = useSelect( ( select ) => {
+		const { getEntityRecord } = select( coreDataStore );
+		const siteData = getEntityRecord( 'root', '__unstableBase', undefined );
+
+		const site_icon_url = getEntityRecord< {
+			site_icon_url: string;
+		} >( 'root', '__unstableBase' )?.site_icon_url;
+
+		return {
+			isRequestingSite: ! siteData,
+			siteIconUrl: site_icon_url,
+		};
+	}, [] );


I guess the use case in this PR doesn't make a great example of why the store coupling is necessary, but if you could provide some examples of what we're planning to do, that could be helpful.

retrofox · 2025-02-12T16:04:01Z

I guess the use case in this PR doesn't make a great example of why the store coupling is necessary, but if you could provide some examples of what we're planning to do, that could be helpful.

I’m planning to refine and clarify the approach in follow-up PRs. For now, this PR (#99348) serves as a proof of concept, showcasing the full implementation in detail.

Breaking it down into smaller pieces is tricky, as the coupling makes it hard to isolate certain parts cleanly. I did attempt it here, but I agree that some aspects might still feel a bit unclear. I’ll keep working on making the plan more digestible.

retrofox self-assigned this Feb 10, 2025

retrofox force-pushed the update/site-admin-introduce-site-icon-core-copy branch from c0d8308 to 0648b4b Compare February 10, 2025 15:00

retrofox changed the title ~~Update/site admin introduce site icon core copy~~ [Site Admin]: introduce <SiteIcon /> component Feb 10, 2025

retrofox force-pushed the update/site-admin-introduce-site-icon-core-copy branch 2 times, most recently from 7915143 to b38b55a Compare February 11, 2025 11:32

retrofox added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 11, 2025

retrofox requested review from psealock and tyxla February 11, 2025 11:40

retrofox marked this pull request as ready for review February 11, 2025 12:14

retrofox requested a review from mirka February 11, 2025 12:26

mirka reviewed Feb 12, 2025

View reviewed changes

retrofox force-pushed the update/site-admin-introduce-site-icon-core-copy branch from e0c2fdb to 5d81c76 Compare February 12, 2025 10:30

retrofox force-pushed the update/site-admin-introduce-site-icon-core-copy branch from 5d81c76 to c3ab459 Compare February 12, 2025 10:53

mirka approved these changes Feb 12, 2025

View reviewed changes

retrofox force-pushed the update/site-admin-introduce-site-icon-core-copy branch 2 times, most recently from f049a49 to f1b1a8b Compare February 12, 2025 13:25

retrofox added 10 commits February 12, 2025 13:47

introduce SiteIcon component

63dc249

add storybook pkgs dependencies

795fa4d

Support storybook for this package

296553e

add package dependencies

7f2f6b1

add Woo logo to the public storybook folder

f28ff49

set public/ as storybook public folder

9f6b9dd

basic core data store mock for storybook

bd60034

minor jsdoc change

c0b8052

add site-icon story

ff05f0d

clean unused impoerted libs

d98e685

tyxla and others added 18 commits February 12, 2025 13:47

Add missing @storybook/addon-toolbars addon

33270a5

rename/add public images to storybook

73da979

add regular and jetpack testing sites

8a7018f

populate mock stories registry

6d4e206

type SiteIcon story args

b34a2c7

add @example to SiteIcon component

2cde4a7

makestory site titles more friendly

e607604

expose SiteIcon component

e907a14

Update packages/site-admin/package.json

c81cecc

Co-authored-by: Lena Morita <[email protected]>

do not export SiteIcon component

924d0f4

do not export the component by defaull. Add jsdoc

4325c0b

do not overwrite styles by using component class

1378cee

use CSF3 format to define story component

1d89cd3

add a todo about reorganizing styles in follow-ups

e9bc9a5

replace Site selector icon with home

a6f7a8c

The description should end with a period

f1d4939

Update packages/site-admin/src/components/site-icon/index.tsx

4064780

Co-authored-by: Lena Morita <[email protected]>

remove unused __() lib

c0d637e

retrofox force-pushed the update/site-admin-introduce-site-icon-core-copy branch from f1b1a8b to c0d637e Compare February 12, 2025 13:48

retrofox merged commit 4f5b912 into trunk Feb 12, 2025
13 checks passed

retrofox deleted the update/site-admin-introduce-site-icon-core-copy branch February 12, 2025 14:36

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 12, 2025

tyxla reviewed Feb 12, 2025

View reviewed changes

retrofox mentioned this pull request Feb 13, 2025

[Site Admin]: Improve SiteIcon component story #99710

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Site Admin]: introduce `<SiteIcon />` component #99540

[Site Admin]: introduce `<SiteIcon />` component #99540

retrofox commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited by matticbot

Loading

matticbot commented Feb 10, 2025

mirka left a comment

mirka Feb 12, 2025

retrofox Feb 12, 2025

tyxla Feb 12, 2025

retrofox Feb 12, 2025

tyxla Feb 12, 2025

retrofox Feb 13, 2025

mirka Feb 12, 2025

retrofox Feb 12, 2025

retrofox commented Feb 12, 2025

mirka commented Feb 12, 2025

mirka left a comment

matticbot commented Feb 12, 2025

tyxla Feb 12, 2025

retrofox Feb 12, 2025

retrofox Feb 13, 2025

tyxla Feb 12, 2025

retrofox Feb 13, 2025

retrofox Feb 13, 2025 •

edited

Loading

tyxla Feb 12, 2025

retrofox commented Feb 12, 2025

	export const createMockRegistry = ( stores = {} ) => {
	export const createMockRegistry = ( stores ) => {

[Site Admin]: introduce <SiteIcon /> component #99540

[Site Admin]: introduce <SiteIcon /> component #99540

Conversation

retrofox commented Feb 10, 2025 • edited Loading

Storybook

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Feb 10, 2025 • edited by matticbot Loading

matticbot commented Feb 10, 2025

mirka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retrofox commented Feb 12, 2025

mirka commented Feb 12, 2025

mirka left a comment

Choose a reason for hiding this comment

matticbot commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retrofox Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retrofox commented Feb 12, 2025

[Site Admin]: introduce `<SiteIcon />` component #99540

[Site Admin]: introduce `<SiteIcon />` component #99540

retrofox commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited by matticbot

Loading

retrofox Feb 13, 2025 •

edited

Loading