[wp-trac] [WordPress Trac] #47048: Nav Menus Admin: Missing Deselect All text when toggling Select All

[WordPress Trac]

#47048: Nav Menus Admin: Missing Deselect All text when toggling Select All
-------------------------------------+-------------------------------------
 Reporter:  birgire                  |       Owner:  audrasjb
     Type:  defect (bug)             |      Status:  accepted
 Priority:  normal                   |   Milestone:  5.3
Component:  Menus                    |     Version:  3.0
 Severity:  normal                   |  Resolution:
 Keywords:  has-screenshots has-     |     Focuses:  ui, accessibility,
  patch                              |  administration
-------------------------------------+-------------------------------------

Comment (by birgire):

 Thanks for a great review @afercia

 > Within attachTabsPanelListeners the variable selectAll is undefined: it
 needs to be declared at the top of the function. Please run grunt jshint
 to lint the JS files

 Thanks for the headup. Usually phpcs and jshint are part of my workflow,
 but sometimes things slip by :-)

 >While it is a partially pre-existing thing, could anyone please refresh
 my mind about the usage of removeAttr( 'checked' ) vs. prop( 'checked',
 false ) and prop( 'checked', true )? Genuinely asking.

 Good question. According to:

 https://jquery.com/upgrade-guide/3.0/#breaking-change-removeattr-no-
 longer-sets-properties-to-false

 >Breaking change: .removeAttr() no longer sets properties to false
 >Prior to jQuery 3.0, using .removeAttr() on a boolean attribute such as
 checked, selected, or readonly would also set the corresponding named
 property to false. This behavior was required for ancient versions of
 Internet Explorer but is not correct for modern browsers because the
 attribute represents the initial value and the property represents the
 current (dynamic) value.
 >
 >It is almost always a mistake to use .removeAttr( "checked" ) on a DOM
 element. The only time it might be useful is if the DOM is later going to
 be serialized back to an HTML string. In all other cases, .prop(
 "checked", false ) should be used instead.

 [attachment:"47048-6.diff"] replaces the {{{.removeAttr( "checked" )}}}
 instances with {{{.prop( "checked", false ) }}}.

 >I'd rename data-area to something along the lines of "data-items-type" or
 something like that to better clarify what it is.

 Done.

 >Originally, the various "Select all" were links. It's a pattern that was
 commonly used in WordPress for no-js compatibility. Progressive
 enhancement: the basic functionality was available also whit JS off (see
 it was using a &selectall=1 param). Now that the "Select All" are
 checkboxes and their functionality is based on JavaScript, when JS support
 is off they're hidden with the CSS class hide-if-no-js. Personally I don't
 have objections to make this feature only available when JS is on but I
 guess the logic to check for $_REQUEST['selectall'] that's still in place
 should be removed?

 [attachment:"47048-6.diff"] replaces that logic regarding
 {{{$_REQUEST['selectall']}}}.

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