[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 has-patch  |     Focuses:
---------------------------------------+-----------------------

Comment (by jorbin):

 Replying to [comment:77 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.

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