Make WordPress Core

Opened 3 years ago

Closed 9 months ago

Last modified 6 months ago

#54370 closed task (blessed) (fixed)

Add an option to configure the site icon in general settings

Reported by: jameskoster's profile jameskoster Owned by: jorbin's profile jorbin
Milestone: 6.5 Priority: normal
Severity: normal Version: 5.9
Component: General Keywords: has-screenshots has-patch commit
Focuses: Cc:

Description

In WordPress 5.9 the Customizer is disabled when using a block theme (unless a plugin re-enables it).

Since the Customizer is currently the only place to set a site icon, this means that once 5.9 lands, if you are using a block theme there will be no way to manage your site icon option.

I propose adding a section to handle this in the General settings section of wp-admin.

Attachments (13)

Screenshot 2021-11-03 at 12.34.17.png (103.5 KB) - added by jameskoster 3 years ago.
Screen Shot 2021-11-08 at 19.02.44.png (123.7 KB) - added by andraganescu 3 years ago.
Current Customizer UI
site-icon-setting.png (57.2 KB) - added by h71 18 months ago.
Site icon settings in the setting page
Screenshot 2024-02-01 at 9.19.47 PM.png (387.4 KB) - added by jorbin 10 months ago.
Screenshot 2024-02-01 at 9.31.23 PM.png (573.5 KB) - added by jorbin 10 months ago.
Expected - Remove button.jpg (41.6 KB) - added by krupajnanda 10 months ago.
Remove link.jpg (176.0 KB) - added by krupajnanda 10 months ago.
Screenshot 2024-02-08 at 5.16.00 PM.png (301.1 KB) - added by jorbin 10 months ago.
Screenshot 2024-02-09 at 5.24.16 PM.png (227.4 KB) - added by jorbin 10 months ago.
site-icon.png (248.7 KB) - added by jameskoster 10 months ago.
site-icon.2.png (133.7 KB) - added by jameskoster 10 months ago.
site-icon.svg (53.7 KB) - added by jameskoster 10 months ago.
site-icon.3.png (223.6 KB) - added by jameskoster 10 months ago.

Change History (141)

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


3 years ago

#2 @audrasjb
3 years ago

Thanks for spotting this @jameskoster!

It sounds like we have two choices here: add this site icon option in any case or add it only when a block theme is used.

#3 @audrasjb
3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.9

This ticket was mentioned in Slack in #core-editor by noisysocks. View the logs.


3 years ago

#5 @jameskoster
3 years ago

Seems fair to make the option available regardless? Otherwise it might be confusing if you want to update your site icon after switching from a block theme to a classic theme.

#6 @andraganescu
3 years ago

there will be no way to manage your site icon option.

In a block theme the we have the site logo block and from the template itself we can change other options. I find it weird that only the site icon is relegated to the options page. Isn't there any room for this to be managed in the site editor?

In the customizer there is some cropping/previewing happening once you select a site icon so this should be accounted for as well in tthe options page, the upload form element is not enough.

#8 @jameskoster
3 years ago

I find it weird that only the site icon is relegated to the options page. Isn't there any room for this to be managed in the site editor?

There is a PR (https://github.com/WordPress/gutenberg/pull/35892) to make it possible to set the Site Icon while working with the Site Logo block. But since the Site Icon doesn't appear on the site itself, a dedicated block would be inappropriate.

Therefore a dedicated site settings interface needs to exist in the Site Editor before we can think about making it possible to edit the Site Icon therein. I think we will need such an interface for managing home page settings and so on, but none of that has been designed yet.

#9 @andraganescu
3 years ago

  • Keywords 2nd-opinion needs-design-feedback added

Am I totally wrong in thinking the PR should be enough? For now block themes can only set site icon to be the same with site logo. That is really not so bad.

By adding a stop gap solution, IMO we just seem to create more cruft:

  • Will the site icon setting in settings remain for classic themes or would it only appear if there is a block theme active?
  • Either way it's not great that we're _adding_ things for temporary reasons.
  • If we add it, and it works for both classic and block themes, we'd have yet another thing which can be achieve in two places.
  • If we add it, and it works for block themes only, it will be confusing when a user saw it there and then it's gone.

Also what will happen to this setting once we do have the option to set it in the site editor? I assume it will remain and continue to set the site icon. But at that time we'd have three different implementations of code that sets the site icon: customizer (works only for classic themes), options page (works for both kinds of themes) and site editor settings page (works only for block themes). The setting itself however is shared across theme kinds, only the interface to set it wildly changes based on context.

So I say let's not add this setting to settings. It should have been added when it was coded in the customizer. Now however, seems to me we're just adding, as I said, cruft.

#10 @jameskoster
3 years ago

Am I totally wrong in thinking the PR should be enough? For now block themes can only set site icon to be the same with site logo. That is really not so bad.

If your block theme happens to utilise the Site Logo block then that PR will make the discrete flow for setting the site icon awkward at best. If no Site Logo block is present then the flow is almost entirely undiscoverable. I'd say that is quite bad.

I don't really see this as a temporary solution, unless you consider the entire wp-admin settings views to be temporary, which is another discussion altogether.

Also what will happen to this setting once we do have the option to set it in the site editor?

You can ask this question about all settings. What about the site name, tagline, etc? You can set those in the Site Editor, so should we remove them from Settings? I would say no, but acknowledge it is a bridge we're not ready to cross just yet. In the mean time we need to ensure that existing Customizer functionality (like setting the Site Icon) is at least moderately discoverable when that feature is disabled.

#11 @kjellr
3 years ago

It seems reasonable to me to place this here, next to the site title + tagline fields. We've traditionally displayed them alongside each other in the Customizer, and if I couldn't find a place for this control in the Site Editor UI, I think Settings > General is the first place I'd look.

#12 @shaunandrews
3 years ago

I agree with Jay; This wouldn't be temporary, nor would it be for block-based themes only.

Requiring people to edit a template (which is likely the only place a logo block would exist, if at all) seems unhelpful and not at all obvious.

#13 @shaunandrews
3 years ago

  • Keywords 2nd-opinion needs-design-feedback removed

@andraganescu
3 years ago

Current Customizer UI

#14 @andraganescu
3 years ago

Excellent then. If a site icon exists should we display the same UI that the Customizer has?

https://core.trac.wordpress.org/raw-attachment/ticket/54370/Screen%20Shot%202021-11-08%20at%2019.02.44.png

#15 @jameskoster
3 years ago

Yeah I think it makes sense to re-use as much as we can 👍

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#17 @webcommsat
3 years ago

While there is further discussion on this ticket to achieve a great solution, adding a note for documentation/ marcomms awareness.
Customizer is often the place where less experienced site users update things like this. If it does not go into 5.9, it would be worth having easily accessible documentation / FAQ on what to do if you are using the block editor and want to add/ edit the site icon.

UPDATE with info from @poena that no one has built a way to re-enable it as yet with the block editor / global styles, that is why the change is needed. Re-adding the customizer menu item was reverted.

Last edited 3 years ago by webcommsat (previous) (diff)

This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.


3 years ago

#19 @mkaz
3 years ago

Thanks @webcommsat - I'm starting to track documentation and added it to the list here:
https://github.com/orgs/WordPress/projects/11/views/4

#20 @poena
3 years ago

Would it be enough, short term, to have link from this settings page to the customizer option?

This ticket was mentioned in Slack in #design by jameskoster. View the logs.


3 years ago

#22 @andraganescu
3 years ago

Investigated this a bit.

The Customizer has the site icon functionality as a control, but not a standalone one unfortunately: it is a special case of the cropped image control, which is itself a special case of the image control.

Reusing that implementation in the settings page is nearly impossible as we suffer from the "bananna -> gorilla holding the bananna -> entire jungle" problem. The UI is also wired to the customizer in terms of interactivity and CSS.

The settings admin section is quite lean. To replicate how setting the site icon behaves a new cropping and preview functionality should be added, which is a lot for one thing, which already exists in the Customizer for classic themes. I have not investigated thorroughly enough to see if there is some simple approach that I miss, but implementing a good UX will add quite a lot of stuff to the general settings part.

Therefore this ticket, IMO, is not just duplicating a feature, but instead a new implementation for a new context of this site icon feature. I believe that to make the most of it we should implement it keeping in mind how to turn this later in whatever Gutenberg will need for its settings panel, so maybe a REST site icon thing is good to have.

Given the above, this would be more worth it if - and I doubt it - these admin settings forms will not be later superseded by Site Editor settings panels and other related functionality. Otherwise it is a temporary solution in terms of usefulness, with potentially a lot of stuff required to be crammed in.

I agree with @jameskoster 's view that we should not remove settings because the site editor can set them. But to duplicate settings because the site editor can't set them yet is a different thing in my mind.

Last edited 3 years ago by andraganescu (previous) (diff)

#23 @jameskoster
3 years ago

But to duplicate settings because the site editor can't set them yet is a different thing in my mind.

The thing is, it isn't a duplication. When a block theme is active the Customizer is hidden, which means the only way to set the site icon is through an awkward flow in the Site Logo block.

This issue seeks to make it easy to manage the site icon when a block theme is active.

Apologies if this is a short-sighted suggestion, but wan we not use something like the Featured Image interface in the classic editor? That allowed you to upload, choose, and edit (crop) images all via the media library modal.

#24 @kjellr
3 years ago

I understand the technical reasons for not wanting to adding this in, but sending folks to the Customizer to edit this single setting is a really confusing user experience. I don't believe we can link directly to just this field in the Customizer, so when they land there, they're going to see two controls they've just navigated away from in the General settings UI (Site Title and Tagline).

Beyond that though, they'll end up in a totally different UI just to modify a single setting that's generally of minor importance. It seems like this would be the only entry-point to the Customizer that's still visible in the UI by default in 5.9, correct? It seems really weird to keep it around just for that.

Last edited 3 years ago by kjellr (previous) (diff)

#25 @shaunandrews
3 years ago

Given the above, this would be more worth it if - and I doubt it - these admin settings forms will not be later superseded by Site Editor settings panels and other related functionality. Otherwise it is a temporary solution in terms of usefulness, with potentially a lot of stuff required to be crammed in.

I don't understand why you think this would be temporary? I foresee the need to have a way to change a site's icon without having to use the visual site editor; Just like someone can edit a site's title or tagline without the need to use the site editor.

Involving the customizer as a solution is temporary. Creating a way to edit a site's icon _without_ the customizer or the site editor is the preferred solution for now, and the foreseeable future.

#26 @stacimc
3 years ago

Creating a way to edit a site's icon _without_ the customizer or the site editor is the preferred solution for now, and the foreseeable future.

I agree with @shaunandrews. The flow to set the Icon through the Site Logo block is not sufficient. The block only has an option to sync the Icon with the Logo. At the moment I have help text which links the user to the customizer (for now) to further edit the icon.

The logic to sync the logo and icon from the block is non-trivial, so if there's interest in moving that PR forward as part of this issue I'd love to get some more eyes on it: https://github.com/WordPress/gutenberg/pull/35892

That complexity is maybe another reason to +1 adding an option elsewhere/temporarily involving the customizer.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

#28 @audrasjb
3 years ago

  • Milestone changed from 5.9 to 6.0

Moving to milestone 6.0 as today is 5.9 beta 1.

This ticket was mentioned in Slack in #core by mkaz. View the logs.


3 years ago

#30 @kafleg
3 years ago

#54630 was marked as a duplicate.

#31 @andraganescu
3 years ago

@shaunandrews I think it will be temporary in terms of usefulness as I am looking at explorations such as this one around surfacing site navigation and combining that with reusable block management, global styles. It's likely many settings will find a home in various panels or via blocks. I wonder for the user of a full block theme what is the point in sending them to the settings screen to update something which will then be reflected across the site in the header and the favicon, and do so saving directly to the live site.

I also believe there is a very small number of "settings" that won't be absorbed by the editor, so the usefulness of having them in a separate unpreviewable form will get ever lower.

#32 @Suresh Shinde
3 years ago

#55487 was marked as a duplicate.

#33 @desrosj
3 years ago

  • Version set to 5.9

#34 @peterwilsoncc
3 years ago

  • Milestone 6.0 deleted
  • Resolution set to reported-upstream
  • Status changed from new to closed

The Gutenberg pull request mentioned earlier in this discussion as an upstream fix has since been merged and is included in WP 5.9.

The site editor links to the relevant page in the customizer to allow admins to set an icon.

As this was reported upstream, I will close it as such. The issue referenced was https://github.com/WordPress/gutenberg/issues/30406

#35 @critterverse
3 years ago

  • Resolution reported-upstream deleted
  • Status changed from closed to reopened

Hey all — just wanted to reopen this issue because I find the current flow of linking to the Customizer to be very weird/confusing from a user experience perspective. Can we look at adding an option to configure the site icon in the General settings section of wp-admin as discussed above?

#36 @SergeyBiryukov
3 years ago

  • Milestone set to Awaiting Review

#37 @clubkert
2 years ago

I find the current flow of linking to the Customizer to be very weird/confusing from a user experience perspective.

I'd like to +1 this comment.

I found this issue because I am writing a guide on how to set the site icon using our FSE theme. For what seems like it should be a simple task, the steps are actually quite complex.

#38 @heymehedi
2 years ago

I think adding the site icon in general settings would be good choice. Hope it will be available soon.

Currently I am changing the icon by switching to other theme, after adding the icon switched back to current theme.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


18 months ago

#40 @kirasong
18 months ago

I find the current flow of linking to the Customizer to be very weird/confusing from a user experience perspective.

I read through the history here after a kind ping from @annezazu and wanted to also add a +1 to this.

Do I understand correctly that currently the only way to modify the Site Icon with a Block Theme is through the Site Logo block?

It seems like there is general consensus that it's a good idea to add it to Settings -> General as well, and I agree that's where I'd go to find it, given the current state of things.

It would be ideal if this were not a link to the Customizer, but I think it would improve things for users even if that turned out to be the case, given the discoverability issues around users finding the Site Logo block method. If we were to go that route, we could remove / change it when someone has the bandwidth to write something new.

In either case, I've messaged the #core-media channel (linked above) to find out if anyone has the bandwidth for any exploration here, or maybe some additional technical ideas on how to approach the problem.

Last edited 18 months ago by kirasong (previous) (diff)

#41 @paaljoachim
18 months ago

It would be great to get some movement in this ticket. As this is one feature that is easy to handle through the customizer but when the customizer is not available as with Full Site themes it right away becomes more difficult. Adding the option and syncing the favicon between the customizer and the WordPress Setting -> General screen would be helpful.

@h71
18 months ago

Site icon settings in the setting page

#42 @h71
18 months ago

Here's my take:

It makes sense to handle the 'Site Icon' and 'Site Title' in the same way. So, adding a way to upload the 'Site Icon' in the main settings could be a good move.

The settings page isn't as visually focused as the customizer, so we don't really need a preview. A simpler solution (considering the implementation complexity) could be to add a file upload button on the settings page, allow for cropping, save the settings, and then check how it looks in the web browser.

On the other hand, this is the first setting that involves media, images, or uploads. I'm not entirely sure if a basic file upload button will be suitable if there will be more similar options in the future.

#43 @jorbin
18 months ago

@h71 That looks pretty good, but I think it might be best to copy over some of the experience from the customizer such as:

1) Instead of an upload button, make it a target that an image can be dropped in OR that you can open up the media library

2) When one is uploaded, we should display it (I like the way the customizer makes use of displaying both the image and a preview of what a tab in the browser will look like) and also the option to remove the icon or the option to replace it.

#44 @jorbin
11 months ago

  • Milestone changed from Awaiting Review to 6.5
  • Owner set to jorbin
  • Status changed from reopened to assigned

My plan is fairly similar to what I mentioned in the previous comment. Hoping to get this in 6.5

#45 @jorbin
10 months ago

A bit of history:

When the site icon functionality was originally added in #16434 the first pass had it on the options screen [32994], however someone advocated for removing it from the options page which was done in [33329].

This does give a good amount of prior art to use here.

#46 @jorbin
10 months ago

Still getting it all wired in, but these screenshots show the the state with and without an icon.

This ticket was mentioned in PR #6064 on WordPress/wordpress-develop by @jorbin.


10 months ago
#47

  • Keywords has-patch added; needs-patch removed

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

This largely brings back code that was removed in https://core.trac.wordpress.org/changeset/33329 but it keeps everything on the same page rather than having a second flow just for site icon. This comes at the cost of no-js mode so the option is hidden from no-js users.

#48 @jorbin
10 months ago

First pass is now up and testing can start.

I'm sure there are bugs, including many that I haven't discovered. Known issues:

  • There is some weirdness when you close out the modal and then restore it if you are in the cropping step. Going to look at how the customizer handles this and replicate that.
  • If you go through the cropping step and then try to update again, the button to select an image in the modal has no text
  • The button for "Choose a site icon" isn't a drag and drop target
  • No capablity checks yet for behavior when a user has manage_options but not media-related options. I think this might be best to hide in that situation, but open to other ideas.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-test by webtechpooja. View the logs.


10 months ago

#51 @krupajnanda
10 months ago

  • Keywords has-screenshots 2nd-opinion added

Hi @jorbin,

I was testing the PR that you had mentioned here -> https://core.trac.wordpress.org/ticket/54370#comment:47

The functionality works as expected but I was thinking if we can incorporate the similar approach for remove option. Instead of "Remove Site Icon" Link, shall we use the button there?

It will help us to achieve the similar UX as the customiser gives.

Attaching the screenshot for the same.

#52 @huzaifaalmesbah
10 months ago

Test Report

Description

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6064

Environment

  • WordPress: 6.5-alpha-56966-src
  • PHP: 8.2.15
  • Server: nginx/1.25.3
  • Database: mysqli (Server: 5.7.44 / Client: mysqlnd 8.2.15)
  • Browser: Chrome 121.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Four 1.0
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.0.1

Actual Results

  1. ✅ Now allow add site icon on General Settings

Additional Notes

  • ❌ But "remove site icon" button doesn't match "change site icon" button UI properly.

Screenshots

https://i.ibb.co/JCBDPHG/Screenshot-2024-02-08-at-10-21-51-PM.png

#53 @jorbin
10 months ago

  • Keywords commit added; 2nd-opinion removed

@krupajnanda Thanks. I had reused the UI from the original implementation in #16434. I've updated it as the above screenshot shows, but I'm not 100% sure that this is better.

Either way, planning to commit this as a v1 tomorrow. UI details can always be iterated on once this lands.

This ticket was mentioned in Slack in #design by paaljoachim. View the logs.


10 months ago

@jorbin commented on PR #6064:


10 months ago
#55

@joedolson Thanks for the review. I think that consistency for casing is also going to involve tweaks to the site editor and the customizer since those are also inconsistent.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/622599/57943d5f-36a3-42ff-9964-bf3e3ee48d53

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/622599/4d4b55ac-4daf-4cf0-8f41-245612684ff3

#56 @afercia
10 months ago

I'm all for this as it finally provides users with a clear, discoverable user interface to manage the site icon. It also provides an prominent, usable, place where to preview the image.

Re: the preview: Worth noting that when the customizer is disabled, the only place where users can preview the Site Icon so far is in the Gutenberg editor UI and that's under discussion in https://github.com/WordPress/gutenberg/issues/57813 as a control of the editor user interface that has a different purpose doesn't seem to be an appropriate place for the preview.

#57 @jorbin
10 months ago

The more I looked at it and used it, the more the duel buttons felt odd, so remove now looks like a link (it's still actually a button since it does an action rather than linking somewhere).

I also updated the preview to better match the customizer since seeing it so large could give a false sense of how something will look to the user.

#58 @huzaifaalmesbah
10 months ago

Looks Great.

https://i.ibb.co/KNnv637/Screenshot-2024-02-10-at-11-18-26-AM.png

@kebbet commented on PR #6064:


10 months ago
#59

I think the design of the remove button can be improved. Maybe the style of button that deletes a theme can be reused, which is red text as non hover, and white text with red background as hover state.

Could be solved by:

  • adding button button-secondary reset to the reset button
  • adding this css to site-icon.css (copied from themes.css)
    .site-icon-section button.reset {
    	color: #b32d2e;
    	text-decoration: none;
    	border-color: transparent;
    	box-shadow: none;
    	background: transparent;
    }
    
    .site-icon-section button.reset:hover {
    	background: #b32d2e;
    	color: #fff;
    	border-color: #b32d2e;
    	box-shadow: 0 0 0 1px #b32d2e;
    }
    

@kebbet commented on PR #6064:


10 months ago
#60

See screenshots for example with and without hover
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/11491369/5bb9296c-2dcc-4983-b1e3-86ae7d32f851
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/11491369/b8b5f57d-b3dd-4931-be6c-df5e23e619bb

#61 @jameskoster
10 months ago

The preview is prone to becoming outdated as browsers evolve, and doesn't capture the full scope of the site icon usage. It might work better in documentation?

In terms of the UI, I'm wondering if we could design something that resembles the 'Media' panel of the Site Logo block more closely?

@kebbet commented on PR #6064:


10 months ago
#62

Small nitpick, but maybe add 10px margin between buttons to give them more space? Otherwise I think it looks ok!

like this:
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/11491369/e18aada2-a821-473f-bfd5-e6ac5117dcc6

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


10 months ago

@kebbet commented on PR #6064:


10 months ago
#64

Tested handheld view, with possible sv_SE translations. Might need some media-queries for smaller viewports. Remove inline-margins and add top margin to reset-button?

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/11491369/ac208ebe-c043-4d80-ad4a-ea2a81e67dde

@kebbet commented on PR #6064:


10 months ago
#65

Tested handheld view, with possible sv_SE translations. Might need some media-queries for smaller viewports. Remove inline-margins and add top margin to reset-button?

Strings:
Ändra webbplatsikon
Ta bort webbplatsikon

#66 @jorbin
10 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57602:

General: Add an option to configure the site icon in general settings.

This restores the site icon setting to its original home on the settings page where it lives with the title and tagline.

The base for this code was originally added in [32994] and then removed in [33329]. The majority of the modification to that version are to remove the no-js pieces and make the workflow completely inline rather than putting the cropping on a separate page.

Additionally, since image crops rely on the ability to upload an image, this setting is gated by the upload_files capability.

Follow-up to: [32994], [33329].

Props jorbin, audrasjb, mukesh27, joedolson, afercia, kebbet, swissspidy, obenland, jameskoster, kjellr, andraganescu, stacimc, mikeschroder, h71, krupajnanda, huzaifaalmesbah.
Fixes #54370.
See #16434.

#67 follow-up: @jorbin
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to encourage continued discussion and potential interaction of the UI.

@jameskoster

The preview is prone to becoming outdated as browsers evolve, and doesn't capture the full scope of the site icon usage.

It hasn't yet in the 9 years that it's been in use in the customizer, so I don't think this is a concern.

In terms of the UI, I'm wondering if we could design something that resembles the 'Media' panel of the Site Logo block more closely?

A couple of issues with that UI (and it might be good to get those fixed up there)

  1. The icon is circle-cropped in the UI even if the icon is not actually circle-cropped.
  2. It's more clicks to complete tasks.
  3. The Site Logo block has the toggle "Use as site icon", the rest of core refers to the feature in Title Case.

@kebbet Thanks, do you want to do a new PR with those changes?

#68 in reply to: ↑ 67 @kebbet
10 months ago

Replying to jorbin:

Reopening to encourage continued discussion and potential interaction of the UI.

@jameskoster

The preview is prone to becoming outdated as browsers evolve, and doesn't capture the full scope of the site icon usage.

It hasn't yet in the 9 years that it's been in use in the customizer, so I don't think this is a concern.

In terms of the UI, I'm wondering if we could design something that resembles the 'Media' panel of the Site Logo block more closely?

A couple of issues with that UI (and it might be good to get those fixed up there)

  1. The icon is circle-cropped in the UI even if the icon is not actually circle-cropped.
  2. It's more clicks to complete tasks.
  3. The Site Logo block has the toggle "Use as site icon", the rest of core refers to the feature in Title Case.

@kebbet Thanks, do you want to do a new PR with those changes?

I might get time tomorrow (in 12h or so), but otherwise a new ticket can be created with follow ups for later, so this can be closed off.

Margins are tricky when stacking. Might need to flex on parent element.

#69 @sabernhardt
10 months ago

  • Keywords commit removed

#70 @jameskoster
10 months ago

It hasn't yet in the 9 years that it's been in use in the customizer, so I don't think this is a concern.

I disagree. It's recognisable as a browser / tab, but visually it looks very outdated. None of the tabs in the three major browsers look like the current preview. Others (e.g. Arc) have gone in a different direction entirely.

The help text / description explain that the icon will appear in browser tabs, which seems adequate for a settings page. Previews capturing the full range of site icon usage can appear in documentation.

It's more clicks to complete tasks.

Which tasks? Replacing is single-click, as is removing.

#71 @jameskoster
10 months ago

If the preview is non-negotiable, then could we use a more abstract svg? This reduces the impact of browser UI evolution a bit, and makes it easy to update the preview based on the active admin theme, and as/when the admin UI itself changes.

This ticket was mentioned in PR #6101 on WordPress/wordpress-develop by @kebbet.


10 months ago
#72

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

This PR ddresses:

  • buttons flow better on small viewports (taking account longer translations)

-alt strings are translateable

  • removed unused ID's

Additional props to @afercia for commenting on original PR.

#73 @kebbet
10 months ago

I just added a follow up PR with minor fixes after [57602], see PR details for info.

#74 @SergeyBiryukov
10 months ago

In 57618:

General: Some minor adjustments for the Site Icon option:

  • Improve buttons flow on small viewports, taking longer translations into account.
  • Make alt attributes translatable.
  • Remove unused id attributes.

Follow-up to [57602].

Props kebbet, afercia, mukesh27.
See #54370.

@SergeyBiryukov commented on PR #6101:


10 months ago
#75

Thanks for the PR! Merged in r57618.

#76 @swissspidy
10 months ago

  • Type changed from enhancement to task (blessed)

Changing to a task so we can refine this during beta.

#77 follow-up: @afercia
10 months ago

  • Keywords needs-patch added; has-patch removed

After a second review pass together with @SergeyBiryukov and @poena it appears there's a few things to address and also a couple bugs to fix.
Personally, and it's only my humble personal opinion, I'm not sure [57602] was sufficiently refined and tested to be committed. It would be great to have a better definition of what is considered 'ready to be committed'.

Also to consider: #60524 to fully address the accessibility part.

Some of the points surfaced during a review, not pretending to be an exhaustive list:

  • Bug: Fix the non-translatable strings. Fixed in [57618].
  • Remove unused id attributes. Fixed in [57618].
  • The accessibility feedback from https://github.com/WordPress/wordpress-develop/pull/6064#pullrequestreview-1872283972 hasn't been addressed.
  • There's a focus loss when removing the image: focus needs to be managed.
  • Fix the missing coding standards in the JS part, mostly:
    • missing spaces, several occurrences
    • it appears there's an unnecessary anonymous function wrapping most o fthe code
    • all var should be declared at the very top of a function block
    • JS globals should be noted at the top of the file e.g. /* global wp, jQuery */
  • Bug: The choose modal frame title doesn't work because there's no data-choose attribute. It should use data-choose-text.
  • Some of the new JS functions miss a docblock and parameters documentation.
  • Also the event callback functions should have a docblock.
  • Fix minor coding standards in the CSS part.
  • Bug: Fix typo in CSS selector that prevents focus style to work as expected. See .button-ad-site-icon:focus with a missing d.

Optimization:

  • Selecting elements with jQuery multiple times should be avoided, for example #site-icon-preview or #choose-from-library-link could be selected only once and assigned to a variable
  • Also, multiple calls to jQuery attr() should be grouped into one call e.g. this two consecutive lines should be refactored:
$( '#choose-from-library-link' ).attr( 'class', $( '#choose-from-library-link' ).attr( 'data-alt-classes' ) );
$( '#choose-from-library-link' ).attr( 'data-alt-classes', classes );

#78 @jorbin
10 months ago

@jameskoster I didn't intend to say that things are non-negotiable, sorry if it came across that way. What I want to avoid is there being three different ways that this is managed which is why the direction right now closely remembers the customizer. It also seems to be the direction you had previously supported.

Stepping back a second, I think that the UI for this needs to get a couple of pieces of information across:

  1. What is a site icon and what do you need to know about adding one (for instance the minimum size)?
  2. What does the site icon look like?
  3. Which site icon is selected?

I think the app icon / faux browser duel preview does a good job with 2&3. I'm not sure that just displaying it in the smaller browser concept will do a good job of helping people understand. What would you think of taking that, but making it a bit shorter so that it and a larger icon would fit next to each other?

I like the direction you went with the Replace/Remove. What do you envision the hover/focus state being for the remove?

Moving the text down to one line also seems a worthy idea, but I think the information about minimum size and shape is still beneficial. How about a single string saying: "Site Icons are what you see in browser tabs, bookmark bars, and within the WordPress mobile apps. Site Icons should be square and at least 512 × 512 pixels."

Last edited 10 months ago by jorbin (previous) (diff)

This ticket was mentioned in PR #6109 on WordPress/wordpress-develop by @kebbet.


10 months ago
#79

  • Keywords has-patch added; needs-patch removed

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

Work In Progess. Feel free to help out.

#80 follow-up: @jameskoster
10 months ago

It also seems to be the direction you had previously supported.

A lot happens in two years :D Specifically; the Site Editor is now more established, and the Customizer fading a bit further into the background. Visually aligning with the former seems a more 'future-proof' strategy, especially given the ongoing admin design work (https://make.wordpress.org/core/2023/07/12/admin-design/).

So I would still advocate for that direction, and suggest adding richer, more comprehensive previews to the site icon documentation (https://wordpress.org/documentation/article/create-a-favicon/).


If we wanted to include the larger, rounded 'home icon' preview too, I'd be tempted to apply a stronger visual treatment to better distinguish the preview from the rest of the UI. But this feels over-emphasised to me.

#81 in reply to: ↑ 77 @jorbin
10 months ago

Replying to afercia:

Personally, and it's only my humble personal opinion, I'm not sure [57602] was sufficiently refined and tested to be committed. It would be great to have a better definition of what is considered 'ready to be committed'.

The definition of 'ready to be committed' is dynamic, with higher standards the closer to final release the commit is being made. During alpha for an enhancement is that something is first going to cause no harm, second, while it might not be in its full final form, the code is ready for additional testing, and third is something that me as the committer is willing to see through to it. It does not mean that this represents the feature in its final form or that the code is of the highest quality.

Remove unused id attributes. Fixed in [57618].

I think removing this was premature and it would be best to add it back and use it for aria-describedby since that will be consistent with the rest of the experience on this page.

The accessibility feedback from https://github.com/WordPress/wordpress-develop/pull/6064#pullrequestreview-1872283972 hasn't been addressed.

It was addressed, even if you don't agree with it and had further feedback after it was committed. I think as the UI gets finalized, it would be good to revisit this while also making sure that the improvements make their way to the customizer.

There's a focus loss when removing the image: focus needs to be managed.

Thanks, I'll set the focus to the choose an icon button upon remove unless you think it belongs elsewhere.

Fix the missing coding standards in the JS part, mostly:
missing spaces, several occurrences

The linting does not detect any issues, but happy to look through it with a finer eye.

it appears there's an unnecessary anonymous function wrapping most o fthe code

This is legacy from when it was two files. Fixing it.

all var should be declared at the very top of a function block

I've fixed the few instances where this wasn't true.

JS globals should be noted at the top of the file e.g. /* global wp, jQuery */

fixed in my latest patch.

Bug: The choose modal frame title doesn't work because there's no data-choose attribute. It should use data-choose-text.

Sorry, I must have misread your review comment about removing this. I'll adjust it.

Some of the new JS functions miss a docblock and parameters documentation.
Also the event callback functions should have a docblock.

As this is not introducing any new API, I prioritized getting to a working version that could be tested over documenting these functions.

Fix minor coding standards in the CSS part.

Patches welcome if you think there is stylistic improvements.

Bug: Fix typo in CSS selector that prevents focus style to work as expected. See .button-ad-site-icon:focus with a missing d.

Thanks, fixed in my patch

Optimization:
Selecting elements with jQuery multiple times should be avoided, for example #site-icon-preview or #choose-from-library-link could be selected only once and assigned to a variable
Also, multiple calls to jQuery attr() should be grouped into one call e.g. this two consecutive lines should be refactored:

I think it's important to not overoptimize with the effect of hurting readability. While I can see the case for caching the selectors, unless there is data showing that there is a large gain, making this code as readable as possible should be a higher priority.

#82 in reply to: ↑ 80 @jorbin
10 months ago

Replying to jameskoster:

It also seems to be the direction you had previously supported.

A lot happens in two years :D Specifically; the Site Editor is now more established, and the Customizer fading a bit further into the background. Visually aligning with the former seems a more 'future-proof' strategy, especially given the ongoing admin design work (https://make.wordpress.org/core/2023/07/12/admin-design/).

So I would still advocate for that direction, and suggest adding richer, more comprehensive previews to the site icon documentation (https://wordpress.org/documentation/article/create-a-favicon/).


If we wanted to include the larger, rounded 'home icon' preview too, I'd be tempted to apply a stronger visual treatment to better distinguish the preview from the rest of the UI. But this feels over-emphasised to me.

This version seems like it would be great to build so that it's easy to see how it will work with a variety of different icons. Do you have a version in figma or something marked up and cut up in such a way that it would be easy to code up?

While it's not as prevelent as it once was, the customizer is still fairly heavily used so improvements shouldn't negelct it. That space is 275px wide, do you think it would be best to cut it shorter on the right there?

sofialeena commented on PR #6109:


10 months ago
#83

Good point, that makes sense.

#84 @afercia
10 months ago

For better coordination: @kebbet is working on a PR to address the missing points reported above, see https://github.com/WordPress/wordpress-develop/pull/6109/
I left some comments there, everyone is welcome to contribute to that PR.

The accessibility feedback from https://github.com/WordPress/wordpress-develop/pull/6064#pullrequestreview-1872283972 hasn't been addressed.

It was addressed, even if you don't agree with it ...

I'm sorry I was not clear then. The feedback is not addressed because the main point is that there's no textual information about the currently selected image. Screen reader users won't have any clue what the selected image is. Alternative text like 'Preview as a browser icon' or 'Preview as an app icon' only explain the visuals of the UI, which I'd say it's not relevant. Instead, there should be some information about the currently selected image.

The linting does not detect any issues, but happy to look through it with a finer eye.

This isn't new. The linter only catches some suntax error. WorfPress never had a tool to check all the JS coding standards as detailed at https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/
It has been discussed since 2016 at least, I think, and there's a Trac ticket open since years but no actual progress so far.

#85 @afercia
10 months ago

I think removing this was premature and it would be best to add it back and use it for aria-describedby since that will be consistent with the rest of the experience on this page.

I'm not sure you are fully getting how aria-describedby is meant to be used. Usually, we use it to associate an input field to a description. In this case, there's no input field. There's buttons that get dynamically hidden. Associating the 'choose' button to the descriptions is technically possible but not recommended, as the button gets hidden when an image is set. I don't think we should use aria-describedby in this UI and thus the IDs are unnecessary. They weren't used anyways in the original commit.

#86 @jameskoster
10 months ago

@jorbin I made a codepen with two size variants here: https://codepen.io/jameskoster/pen/WNmLmzq?editors=1100. Obviously the images should be swapped accordingly, and ideally all the colors should use css vars. Feel free to adjust as required.

#87 follow-up: @andraganescu
10 months ago

I would like to chime in my support fot the idea to not represent the visuals using a browser at all. Mainly because browsers have a revamped appeal as a product and we're in a new proliferation of options and whatever we place here will most likely not match UIs of up and coming products.

Also, just as before, there is a tendency to favor OSX ui, which for Windows users is uncanny as a preview. Also favicon is today less about the tabs and more about the add to home screen, add to dock, add to taskbar options.

#88 in reply to: ↑ 87 @jorbin
10 months ago

Replying to afercia:

I think removing this was premature and it would be best to add it back and use it for aria-describedby since that will be consistent with the rest of the experience on this page.

I'm not sure you are fully getting how aria-describedby is meant to be used. Usually, we use it to associate an input field to a description. In this case, there's no input field. There's buttons that get dynamically hidden. Associating the 'choose' button to the descriptions is technically possible but not recommended, as the button gets hidden when an image is set. I don't think we should use aria-describedby in this UI and thus the IDs are unnecessary. They weren't used anyways in the original commit.

Going based off https://www.w3.org/TR/WCAG20-TECHS/ARIA1.html, this seems as though the exact case that the third example provides "Using aria-describedby property to provide more detailed information about the button"

Replying to andraganescu:

I would like to chime in my support fot the idea to not represent the visuals using a browser at all. Mainly because browsers have a revamped appeal as a product and we're in a new proliferation of options and whatever we place here will most likely not match UIs of up and coming products.

For desktop users, favicons are still the norm and an important part of a site's identity. While arc does go in alternative directions with the UI, it still uses favicons as a part of the site identity. This exemplifies why it's important for there to be both icons, though with PWA never having really gotten going in iOS, I don't know how much saving to home screens actually takes place.

I believe the next steps are:

  1. Finish up the work ongoing in https://github.com/WordPress/wordpress-develop/pull/6109
  2. Wire in the new UI both on the settings screen and in the Customizer.
  3. Update the alt text both on the settings screen and in the Customizer.
  4. Update the link in the site icon block so that it points to the correct place. (Also should fix the casing of Site Icon there so that it is consistent throughout WordPress)

@kebbet commented on PR #6109:


10 months ago
#89

I have tested the PHP-part on a multisite install, and correct alternative text is selected for the preview images.

@afercia commented on PR #6109:


10 months ago
#90

Thanks for working on crafting a more meaningful alt text. The alt text value looks good to me and it's consistent with the pattern already in use for the media widgets and the Gutenberg featured image. To recap:

When the image does have an alt text:
Current image: %s where %s is the alt text.
Otherwise:
The current image has no alternative text. The file name is: %s where %s is the image filename.

However, if we want to keep the current design, there's one more thing to address. While the featured image is only _one_ image, the site icon UI uses _two_ images: one for previewing the site icon as it is used by most browsers in the browser tab and another one for previewing the site icon as it is used by mobile devices, as an 'app icon'.

I think it would be good to consider to:

  • Either distinguish the two image, by prefixing something like 'Browser icon preview' and 'App icon preview'
  • Or provide the alt text only once, by describing both previews only once (technical details to be decided)

@alexstine I'd appreciate your feedback here, keeping in mind that sighted screen reader users do see two images.

@alexstine commented on PR #6109:


10 months ago
#91

@afercia

Either distinguish the two image, by prefixing something like 'Browser icon preview' and 'App icon preview'

I like this approach.

@jorbin commented on PR #6109:


10 months ago
#92

However, if we want to keep the current design, there's one more thing to address. While the featured image is only one image, the site icon UI uses two images: one for previewing the site icon as it is used by most browsers in the browser tab and another one for previewing the site icon as it is used by mobile devices, as an 'app icon'.

See https://core.trac.wordpress.org/ticket/54370#comment:86 for a codepen with the updated design. While there are visual updates, this suggestion still stands since there are two images.

@kebbet commented on PR #6109:


10 months ago
#93

If someone wants to continue the work on this and update the feature with the suggested new design, feel free to take over, this PR now fixes whats currently in trunk.

@jorbin commented on PR #6109:


10 months ago
#94

@kebbet Thank you! I'll handle getting the updated design wired in!

@kebbet commented on PR #6109:


10 months ago
#95

It is no longer possible to crop the image. Why was the cropping dependency remove here 80fa6d0?

@kebbet commented on PR #6109:


10 months ago
#96

@aaronjorbin CSS colors should be all lowercase, 0 should be unitless, see https://developer.wordpress.org/coding-standards/wordpress-coding-standards/css/#properties

@afercia commented on PR #6109:


10 months ago
#97

It is no longer possible to crop the image. Why was the cropping dependency remove here 80fa6d0?

Latest commits broke hte crop step for me. This is what I get in the media modal dialog in the 'crop' step: missing crop UI and the site icon preview is displayd in the sidebar.... 😱 No JS errors.

https://github.com/WordPress/wordpress-develop/assets/1682452/b935fa0e-f7f4-4647-8358-b1396832e81a

@afercia commented on PR #6109:


9 months ago
#98

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:

https://github.com/WordPress/wordpress-develop/assets/1682452/8e8d2d35-a215-41ea-8d28-2252af7f590f

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:

https://github.com/WordPress/wordpress-develop/assets/1682452/fd6c23e6-02ad-4c2b-9bb5-05ad64b82784

@afercia commented on PR #6109:


9 months ago
#99

This pull request was created to address some bugs and other issues after https://core.trac.wordpress.org/changeset/57602
Specifically:

We're not in Beta 2 and new changes were added with the only purpose of updating the design. To me, these additional changes are outside of the scope of this PR. More importantly:

  • We're now in Beta 2.
  • These additional changes aren't bugs, they are enhancements and should not be committed during Beta 2.
  • These changes completely broke the media modal dialog 'crop' step.
  • These changes don't follow the coding standards.

I'd recommend to:

  • Revert the latest changes.
  • Fix the layout of the two previews in the media modal dialog: they shoudl be stacked vertically. This seems a trivial fix as it just needs more specificity for the CSS selectors.
  • Review and test thoroughly.
  • Commit.

In my opinion, any other change should go in a separate pull request or patch after this PR has been committed, to better evaluate if additional changes can be made during Beta 2.

@jorbin commented on PR #6109:


9 months ago
#100

It is no longer possible to crop the image. Why was the cropping dependency remove here 80fa6d0?

The cropping was broken due to some CSS specification issues that I just fixed. jCrop has long been unused by core and wasn't actually used here (It was a part of the original code from [32994] and I hadn't removed it until now)

@jorbin commented on PR #6109:


9 months ago
#101

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

@kebbet commented on PR #6109:


9 months ago
#102

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:

https://private-user-images.githubusercontent.com/1682452/306580028-8876bb0c-e313-4e5a-b7f9-1c25322d3d1e.png

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:

https://private-user-images.githubusercontent.com/1682452/306579760-fd6c23e6-02ad-4c2b-9bb5-05ad64b82784.png

This needs to be addressed.

@kebbet commented on PR #6109:


9 months ago
#103

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 commented on PR #6109:


9 months ago
#104

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?

#105 @jorbin
9 months ago

@jameskoster - There are are handful of open questions on the PR https://github.com/WordPress/wordpress-develop/pull/6109 related to the new design that it would be good to get you to chime in on. They basically fall into two main categories:

  1. Questions about how specific to follow your code pen and if the intent is to be using highly specific measurements (such as 9.824px for a border radius) and multiple box shadows
  1. If the background works well in situations where there is a solid color (or near solid color) Site Icon. It seems like it could cause more confusion then benefit in those situations, so there are a handful of ideas for handling it on the PR.

@kebbet commented on PR #6109:


9 months ago
#106

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

https://github.com/WordPress/wordpress-develop/assets/11491369/0b641c74-7d35-4717-a852-ef4dcfce9922
https://github.com/WordPress/wordpress-develop/assets/11491369/46fedb41-370c-41a7-8a09-7fa14e323883
https://github.com/WordPress/wordpress-develop/assets/11491369/539e25ef-365b-4969-a54f-c430c91ed08c
https://github.com/WordPress/wordpress-develop/assets/11491369/8714a51d-d413-4fd4-a90a-d4caff473066

RTL
https://github.com/WordPress/wordpress-develop/assets/11491369/6145b0af-d3e2-4daf-957f-cf6efe625068

@afercia commented on PR #6109:


9 months ago
#107

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.

#108 @jameskoster
9 months ago

Questions about how specific to follow your code pen and if the intent is to be using highly specific measurements (such as 9.824px for a border radius) and multiple box shadows

The measurements for radius are based on Apple home icons. It's probably fine to round up, though I don't see a need? Of course, these values are subject to change, but that's the nature of using a preview, and why I'd continue to advocate not doing so.

I don't see an issue with multiple box shadow values, but if that's going to be a problem then maybe omit shadows altogether and increase blur on the backdrop.

It seems like it could cause more confusion then benefit in those situations

That's good feedback. I've made an updated codepen (https://codepen.io/jameskoster/pen/abMrYmX) that removes the shadows and improves the appearance when a solid-color site icon is used. This starts to feel like optimising for edge cases, but if we're 100% set on the preview then these things must be considered.

A separate note on the code, ideally all the greys use css variables (assuming those are available outside of Gutenberg) rather than being hard-coded.

#109 @afercia
9 months ago

ideally all the greys use css variables (assuming those are available outside of Gutenberg) rather than being hard-coded.

They are not available. In core, only the About page, bundled themes, and the admin-bar.css (only 1 occurrence), use css variables.

sofialeena commented on PR #6109:


9 months ago
#110

Good point, that makes sense.

@swissspidy commented on PR #6109:


9 months ago
#111

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?

#112 @swissspidy
9 months ago

#60606 was marked as a duplicate.

@jorbin commented on PR #6109:


9 months ago
#113

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:

  1. Rip out the new design and restore the battle-tested UI that is already in place in the customizer.
  2. Revisit the UI in 6.6. With any major desired UI changes coming well before beta 1.
  3. 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!

This ticket was mentioned in PR #6165 on WordPress/wordpress-develop by @jorbin.


9 months ago
#114

This is a copy of #6109 but with the existing UI rather than a new one.

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

@swissspidy commented on PR #6109:


9 months ago
#115

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.

@jorbin commented on PR #6109:


9 months ago
#116

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: https://github.com/WordPress/wordpress-develop/pull/6165/commits/984a20ca6ddae6e945838b207a22da0048fe599a is the commit that restores the old UI. Reviews of that are welcome; it's more red than green.

#117 @jameskoster
9 months ago

May I once again suggest trying a design like this one: https://core.trac.wordpress.org/ticket/54370#comment:61 ?

It's much simpler to implement and maintain, aligned with the site editor design direction, and less prone to edge case quirks and becoming visually outdated.

#118 @swissspidy
9 months ago

Given how late we are in the cycle, I agree with @jorbin that a new design like this is best discussed in a new ticket for 6.6. The design would need to be updated in the customizer as well. Would you mind opening a new ticket for tracking all that?
So revisiting the design makes sense as an enhancement, it's just that it's better done in the next release.

#119 @kebbet
9 months ago

I just created #60625 for a design update of the site icon selection UI.

#120 @jorbin
9 months ago

  • Keywords commit added

https://github.com/WordPress/wordpress-develop/pull/6165 is ready for commit if anyone wants to give it a final review. If I haven't heard anything (a request for additional time would count as something), I will be committing it on Monday so that it is available for the next beta.

#121 @kebbet
9 months ago

Thanks for bringing this closer to a commit @jorbin and @swissspidy!

Testing functionallity: looks good and works as expteced. Alt texts are displayed with correct content on page load and after selection in modal. Cropping works, and the preview in the modal looks ok. RTL works to.

Looking at the code: looks ok from my point of view. Only nitpick is this comment, that stands out in it's tone.
Start over with a frame that is so fresh and so clean clean.

Otherwise ok, and I like the direction with a new ticket for design update over all. The preview looks a bit dated.

#122 follow-up: @jorbin
9 months ago

@kebbet @swissspidy That comment is a reference to Outkast. In my eyes, it's in line with the jovial nature of many of our inline comments and test strings but if it's something either of you view as problematic, I can remove it when I commit it.

#123 @jameskoster
9 months ago

For 6.5 could we update the preview screenshot so that it resembles more contemporary software?

#124 @swissspidy
9 months ago

The preview would need to be updated in the customizer as well and as an enhancement is both outside the scope of this ticket and the 6.5 release, as we're past the enhancement stage. Bug fixes only from now on. But we can address this in #60625 for 6.6.

#125 in reply to: ↑ 122 @kebbet
9 months ago

Replying to jorbin:

@kebbet @swissspidy That comment is a reference to Outkast. In my eyes, it's in line with the jovial nature of many of our inline comments and test strings but if it's something either of you view as problematic, I can remove it when I commit it.

Don't change it. I didn't get the reference this time, and who knows what lyrics I will suggest next time! Happy commiting!

#126 @jorbin
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57713:

Site icon: Polish up Site Icon on the general settings screen.

This fixes a number of issues, chief among them:

  • Updates to the site title are reflected in the preview.
  • Improve alt text for preview
  • Make string describing site icon more succinct.
  • Add inline documentation to JavaScript

Props kebbet, jorbin, swissspidy, afercia, mukesh27, alexstine, jameskoster, andraganescu.
Fixes #54370.

#127 @jorbin
9 months ago

Thanks to everyone who helped us get this here. If there are any other issues, let's open up new tickets for them.

#60641 has been created to port the alt text improvements to the customizer.

This ticket was mentioned in Slack in #design by joen. View the logs.


6 months ago

Note: See TracTickets for help on using tickets.