54370 site icon post review by kebbet · Pull Request #6109 · WordPress/wordpress-develop

kebbet · 2024-02-14T07:54:30Z

https://core.trac.wordpress.org/ticket/54370

Work In Progess. Feel free to help out.

github-actions · 2024-02-14T08:24:01Z

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.

src/js/_enqueues/admin/site-icon.js

src/wp-admin/css/forms.css

src/wp-admin/options-general.php

src/js/_enqueues/admin/site-icon.js

Co-authored-by: Erik <11491369+kebbet@users.noreply.github.com>

aaronjorbin · 2024-02-21T18:01:41Z

Reverting and moving backward won't help get this in the best shape for the final release. Cropping is now fixed. Going to spend some time today getting things finalized for commit

…4370-site-icon-post-review

kebbet

Some notes on the current state of the PR.

kebbet · 2024-02-21T18:03:23Z

src/wp-admin/css/site-icon.css

+.site-icon-preview-inline.settings .home-icon {
+	width: 58px;
+	height: 58px;
+	border-radius: 12.95px;


Can all pixel values be changed to be integers?

kebbet · 2024-02-21T18:03:44Z

src/wp-admin/css/site-icon.css

+	width: 200%;
+	height: 200%;
+	transform: translateX(-25%) translateY(-25%);
+	/* background: linear-gradient(0deg, rgba(0, 0, 0, 0.2) 0%, rgba(0, 0, 0, 0.3) 100%), var( --site-icon-url ) 50%/cover no-repeat; */


Remove unused code?

kebbet · 2024-02-21T18:05:01Z

src/wp-admin/css/site-icon.css

+	flex-shrink: 0;
+	z-index: 1;
+	border-radius: 9.824px;
+	box-shadow: 0 1.517px 3.034px 0 rgba(0, 0, 0, 0.1), 0 4.552px 4.552px 0 rgba(0, 0, 0, 0.09), 0 10.621px 6.828px 0 rgba(0, 0, 0, 0.05), 0 19.724px 7.586px 0 rgba(0, 0, 0, 0.01), 0 30.345px 8.345px 0 rgba(0, 0, 0, 0);


Can box-shadows be simplifed to use one shadow statement, this is really messy.

kebbet · 2024-02-21T18:05:09Z

src/wp-admin/css/site-icon.css

+	border-top: 1px solid rgba(255, 255, 255, 0.2);
+	border-left: 1px solid rgba(255, 255, 255, 0.2);
+	background: linear-gradient(180deg, #dcdcde 0%, #bdbdbd 100%);
+	box-shadow: 0 10px 22px 0 rgba(0, 0, 0, 0.2), 0 40px 40px 0 rgba(0, 0, 0, 0.17), 0 89px 53px 0 rgba(0, 0, 0, 0.1), 0 158px 63px 0 rgba(0, 0, 0, 0.03), 0 247px 69px 0 rgba(0, 0, 0, 0);


kebbet · 2024-02-21T18:05:15Z

src/wp-admin/css/site-icon.css

+	flex: 1 0 0;
+	border-radius: 4px;
+	background: #f6f7f7;
+	box-shadow: 0 1px 3px 0 rgba(0, 0, 0, 0.04), 0 5px 5px 0 rgba(0, 0, 0, 0.03), 0 12px 7px 0 rgba(0, 0, 0, 0.02), 0 22px 9px 0 rgba(0, 0, 0, 0.01), 0 34px 9px 0 rgba(0, 0, 0, 0);


kebbet · 2024-02-21T18:11:01Z

src/wp-admin/css/site-icon.css

+	border-radius: 12.95px;
+}
+
+.site-icon-preview-inline.settings:after {


This background is not updated when a new icon is selected. Use an img-tag or similar instead? And when using solid colors as site icons it is hard to see the icon. Maybe add a opacity to this background, or even skip it.
And using a black site icon makes the shadow disappear.

kebbet · 2024-02-21T18:11:15Z

src/wp-admin/css/site-icon.css

+	height: 44px;
+	flex-shrink: 0;
+	z-index: 1;
+	border-radius: 9.824px;


Make it 10px

kebbet · 2024-02-21T18:13:31Z

src/wp-admin/options-general.php

 		<?php
 			/* translators: %s: Site Icon size in pixels. */
-			printf( __( 'Site Icons should be square and at least %s pixels.' ), '<strong>512 &times; 512</strong>' );
+			printf( __( 'Site Icons are what you see in browser tabs, bookmark bars, and within the WordPress mobile apps. They should be square and at least %s pixels.' ), '<strong>512 &times; 512</strong>' );


Maybe wrap size string in a <code>-tag instead to align with other descriptive texts on the page?

kebbet · 2024-02-21T18:18:00Z

In src/wp-admin/css/site-icon.css, the CSS rulesets with the following selectors target also elements within the media modal dialog in the crop step, thus breaking the layout:
* .site-icon-preview .favicon-preview

* .site-icon-preview .favicon, .site-icon-preview .browser-title

* .site-icon-preview
This is how the media modal dialog looks like on current trunk:

Notice the layout of hte previews in the sidebar is broken.

This is how it is supposed to look, screenshot from WordPress 6.4: notice the previews are separated and vertically stacked:

This needs to be addressed.

kebbet · 2024-02-21T18:23:19Z

Reverting and moving backward won't help get this in the best shape for the final release. Cropping is now fixed. Going to spend some time today getting things finalized for commit

Thanks for clarifying and fixing cropping.

kebbet · 2024-02-21T18:28:20Z

First, have a look at the admin bar when the Site Title option is changed in options-general.php. Is it possible to use the same mechanism in the preview of the site icon? Basically update the tab name, while typing in the Site Title field above?

src/wp-admin/options-general.php

…review in the cropper

…4370-site-icon-post-review

kebbet · 2024-02-22T09:07:58Z

With the latest changes from me, this is how the section looks like

RTL

afercia · 2024-02-22T11:51:22Z

Reverting and moving backward won't help get this in the best shape for the final release.

I kindly disagree. The design changes are outside of the scope of this PR. I'm not sure insisting broadening the scope of this PR is ideal. More importantly, given we're in Beta 2, I'm not sure it's any wise. At this point of the release cycle any change should be as small and self-contained as possible and only focused to fix bugs.

While I agree that updating the design with a more modern browser interface is a nice to have, that's not required for this functionality to work. Also, if updating the design of the preview is desired, I'd like to remind everyone that it would need to be be updated also in other two places;

in the media modal dialog sidebar
in the customizer

I'm not sure providing a different design only in the General Settings page would be ideal and help users as it would be clearly inconsistent and confusing. Extending the design changes to the media modal and customizer in this PR would be really out of scope and make this PR way larger than it was meant to be.

Lastly, @kebbet reported valid concerns about the latest changes that, in my opinion, would need more time to be addressed. As I see it, Beta 2 is not the best timing for such changes. I'd like to make clear that I'm nog against a design update, but that should go in a separate PR so that such changes can be better evaluated to establish if they fit with the release cycle process.

swissspidy · 2024-02-22T13:52:30Z

Hi all 👋

Just chiming in here since I see this ticket / PR has been open for a while now with a lot of activity, so I am just wondering if there's anything I can help with to ensure this is in a good shape to land in time for the next beta.

Could someone perhaps summarize the goals and contents of this pull request for me, maybe with a before/after comparison? Even just updating the PR title & description would be helpful.

Or maybe put another way: what are the issues with the current version in trunk that should be addressed in this release?

Thanks in advance 🙏

In the meantime, since I saw a comment about coding standards, missing whitespace, etc.: Please note that we have tools like Prettier, just use e.g. npx prettier --write src/js/_enqueues/admin/site-icon.js to format code according to the WP coding standards. Works with CSS too :)

Also, if the changes in the PR are too big to review in one go, perhaps they could be split into multiple PRs?

npx prettier@npm:wp-prettier@latest --write src/js/_enqueues/admin/site-icon.js npx prettier@npm:wp-prettier@latest --write src/wp-admin/css/site-icon.css

aaronjorbin · 2024-02-22T19:35:40Z

I've gone back and forth on this for the last few hours. On one hand, the new design more closely resembles the block editor and the current browser UI is outdated. On the other, the new design does not feel ready as it came in very late in the release cycle and as James notes, it hasn't considered edge cases. The improvements that are reflected in the screenshots seem to resolve the known issues, but I'm unsure. I also worry about integrating in this in three different places and then doing further interaction.

As I've mentioned multiple times, I think what's going to provide the best experience for the users of WordPress is an experience that is consistent across the different UIs. Iterating on the UI in the three different places is cumbersome at best and bug-inducing at worst.

Unless there is an objection, I think the path forward is:

Rip out the new design and restore the battle-tested UI that is already in place in the customizer.
Revisit the UI in 6.6. With any major desired UI changes coming well before beta 1.
Port the alt text and description improvements to the customizer in a separate ticket. I would consider this a bug fix for a feature in 6.5 even though that UI isn't directly being touched(@swissspidy, would you agree?)

RE: Prettier. That command doesn't format correctly. I used
npx prettier@npm:wp-prettier@latest --write src/js/_enqueues/admin/site-icon.js and npx prettier@npm:wp-prettier@latest --write src/wp-admin/css/site-icon.css. Thanks!

swissspidy · 2024-02-22T20:41:56Z

Thanks for the update & context @aaronjorbin! That sounds like a sensible path forward to me.

I see you just opened #6165 in the meantime. Would you like the group to review that one now? Just noting it's still in draft.

aaronjorbin · 2024-02-22T20:48:19Z

I opened that PR so that it's ready if this path forward is desired without further clouding this one. The only differences are

1: I fixed the jshint issues that prettier caused 🤦
2: 984a20c is the commit that restores the old UI. Reviews of that are welcome; it's more red than green.

kebbet added 6 commits February 14, 2024 08:18

Fix typo

cb96657

Adds data-choose attribute for media modal

cfffe29

Adds docblocks, removes wrapping function.

ff3ef3a

Adds since tag, follow coding standards I guess.

44ca10d

Store selected elements in vars.

ed9df2c

Updates docblock, move frame into initializing function

54a0565

Reduce calls to .attr(). Remove classes var

e584698

afercia reviewed Feb 14, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

afercia reviewed Feb 14, 2024

View reviewed changes

src/wp-admin/css/forms.css Outdated Show resolved Hide resolved

afercia reviewed Feb 14, 2024

View reviewed changes

src/wp-admin/options-general.php Show resolved Hide resolved

afercia reviewed Feb 14, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

afercia reviewed Feb 14, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

afercia reviewed Feb 14, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

kebbet added 4 commits February 14, 2024 17:18

Adds space before the opening brackets

233f89b

Reuse existing attribute, rename selector (in PHP)

8ffc136

Address review comments from @afercia

5a97e40

Merge branch 'WordPress:trunk' into 54370-site-icon-post-review

a3020b6

mukeshpanchal27 reviewed Feb 15, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

mukeshpanchal27 reviewed Feb 15, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

kebbet added 2 commits February 15, 2024 08:58

JS-fixes

c930802

Focus loss and fixed button ID for triggering modal.

c695e3c

afercia reviewed Feb 15, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

Removes unnecessary setting of focus

564cdbd

afercia reviewed Feb 15, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

afercia reviewed Feb 15, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

afercia reviewed Feb 15, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

afercia reviewed Feb 15, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

afercia reviewed Feb 15, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

afercia reviewed Feb 15, 2024

View reviewed changes

src/js/_enqueues/admin/site-icon.js Outdated Show resolved Hide resolved

aaronjorbin and others added 5 commits February 21, 2024 12:52

Use unitless for 0

138f269

Co-authored-by: Erik <11491369+kebbet@users.noreply.github.com>

lowercase colors

952bb3f

Co-authored-by: Erik <11491369+kebbet@users.noreply.github.com>

Use unitless for 0

4ed3128

Co-authored-by: Erik <11491369+kebbet@users.noreply.github.com>

Use unitless for 0

5ffd471

Co-authored-by: Erik <11491369+kebbet@users.noreply.github.com>

Fix indentation

7b67b64

Merge branch 'trunk' of github.com:WordPress/wordpress-develop into 5…

f023335

…4370-site-icon-post-review

kebbet commented Feb 21, 2024

View reviewed changes

src/wp-admin/options-general.php Outdated Show resolved Hide resolved

aaronjorbin and others added 7 commits February 21, 2024 23:50

Update Title in preview as it chagnes

254a288

Update background for preview with the new image

57f3d4a

Remove float which was used in old UI as it is interfering with the p…

6564f9b

…review in the cropper

Merge branch 'trunk' of github.com:WordPress/wordpress-develop into 5…

69416b4

…4370-site-icon-post-review

Restore alt text for browser and remove svg from screen reader

b76e500

Use <code> tag for size info.

16a3a8c

Tidy CSS and add border to preview field.

91fb8a8

WordPress deleted a comment from sofialeena Feb 22, 2024

use prettier to format CSS and JS

9b68ef5

npx prettier@npm:wp-prettier@latest --write src/js/_enqueues/admin/site-icon.js npx prettier@npm:wp-prettier@latest --write src/wp-admin/css/site-icon.css

aaronjorbin mentioned this pull request Feb 22, 2024

Improve site icon but don't change the UI #6165

Closed

kebbet closed this Aug 11, 2024

kebbet deleted the 54370-site-icon-post-review branch August 11, 2024 07:21

Conversation

kebbet commented Feb 14, 2024

Uh oh!

github-actions bot commented Feb 14, 2024

Test using WordPress Playground

Some things to be aware of

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaronjorbin commented Feb 21, 2024

Uh oh!

kebbet left a comment

Choose a reason for hiding this comment

Uh oh!

kebbet Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

kebbet Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

kebbet Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

kebbet Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

kebbet Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

kebbet Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kebbet Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

kebbet Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

kebbet commented Feb 21, 2024

Uh oh!

kebbet commented Feb 21, 2024

Uh oh!

kebbet commented Feb 21, 2024

Uh oh!

Uh oh!

kebbet commented Feb 22, 2024

Uh oh!

afercia commented Feb 22, 2024

Uh oh!

swissspidy commented Feb 22, 2024

Uh oh!

aaronjorbin commented Feb 22, 2024

Uh oh!

swissspidy commented Feb 22, 2024

Uh oh!

aaronjorbin commented Feb 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kebbet Feb 21, 2024 •

edited

Loading