[wp-trac] [WordPress Trac] #54370: Add an option to configure the site icon in general settings

[WordPress Trac]

#54370: Add an option to configure the site icon in general settings
-----------------------------------------+-----------------------
 Reporter:  jameskoster                  |       Owner:  jorbin
     Type:  task (blessed)               |      Status:  reopened
 Priority:  normal                       |   Milestone:  6.5
Component:  General                      |     Version:  5.9
 Severity:  normal                       |  Resolution:
 Keywords:  has-screenshots needs-patch  |     Focuses:
-----------------------------------------+-----------------------
Changes (by afercia):

 * keywords:  has-patch has-screenshots => has-screenshots needs-patch


Comment:

 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 );
 }}}

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/54370#comment:77>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform