WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 22 months ago

#20261 closed enhancement (fixed)

The 'check all' checkboxes don't get checked/unchecked when appropriate

Reported by: johnbillion Owned by: markjaquith
Milestone: 3.5 Priority: normal
Severity: minor Version: 3.3.1
Component: Administration Keywords: has-patch ux-feedback
Focuses: Cc:

Description

Steps to reproduce:

  1. Click the check box at the top of the checkbox column on any list screen (posts, pages, users, plugins, etc).
  2. Uncheck one of the individual row checkboxes.
  3. The check box at the top of the checkbox column doesn't get unchecked as you might expect.

Attachments (9)

20261-1.diff (1.1 KB) - added by jkudish 2 years ago.
checks/unchecks the "all" checkbox upon checking of any other checkbox (according to the state of all the checkboxes)
20261-2.diff (1.0 KB) - added by jkudish 2 years ago.
adjusted the comment above the function so it's more fitting to what it actually does
20261-3.diff (1.0 KB) - added by jkudish 23 months ago.
revised patch for readability and efficiency
20261-4.diff (398 bytes) - added by SergeyBiryukov 23 months ago.
20261-5.diff (1.2 KB) - added by jkudish 23 months ago.
20261-6.diff (1.1 KB) - added by jkudish 23 months ago.
remove left-over console.log()
20261-7.diff (1.1 KB) - added by jkudish 23 months ago.
use $current_checkbox for $current_table selector
20261-8.diff (1.0 KB) - added by jkudish 23 months ago.
remove svn:ignore that was accidentally included in the previous patch
20261-9.diff (560 bytes) - added by SergeyBiryukov 22 months ago.

Download all attachments as: .zip

Change History (26)

comment:1 scribu2 years ago

  • Keywords needs-patch added

comment:2 johnbillion2 years ago

Personally I don't think the indeterminate state is appropriate for a 'check all' checkbox, but it'd be interesting to see what the consensus is among other apps.

jkudish2 years ago

checks/unchecks the "all" checkbox upon checking of any other checkbox (according to the state of all the checkboxes)

comment:3 jkudish2 years ago

  • Cc joachim.kudish@… added
  • Keywords has-patch dev-feedback needs-testing ux-feedback added; needs-patch removed

jkudish2 years ago

adjusted the comment above the function so it's more fitting to what it actually does

comment:4 jkudish2 years ago

Any reason this couldn't make it into 3.4?

comment:5 follow-up: johnbillion2 years ago

It's low priority, it needs UX feedback, it needs testing, and 3.4 is past feature freeze.

comment:6 in reply to: ↑ 5 jkudish2 years ago

Replying to johnbillion:

It's low priority, it needs UX feedback, it needs testing, and 3.4 is past feature freeze.

Good point. Early 3.5 then :)

comment:7 jkudish23 months ago

  • Keywords dev-feedback removed
  • Severity changed from minor to normal

Reviewed/revised my patch against the latest trunk and revised it for readability and efficiency. Would love to see it in 3.5-early :)

jkudish23 months ago

revised patch for readability and efficiency

SergeyBiryukov23 months ago

comment:8 SergeyBiryukov23 months ago

  • Severity changed from normal to minor

Seems we don't need to iterate through all the checkboxes. Getting a status of the current clicked one is enough (20261-4.diff).

comment:9 SergeyBiryukov23 months ago

Ah, I only tested unchecking, as per the description of the ticket.

If we want to check them automatically as well, would still need to iterate.

comment:10 SergeyBiryukov23 months ago

  • Keywords 2nd-opinion added; needs-testing removed

comment:11 follow-up: dd3223 months ago

A combination could be used.

If the action is uncheck, skip iteration, we know not all are checked.

We can also shift the iteration into jQuery itself, by using the :checkbox :not(:checked) selector to select any checkboxes that are NOT checked, If empty, they're all selected.

jkudish23 months ago

comment:12 in reply to: ↑ 11 jkudish23 months ago

Replying to dd32:

A combination could be used.

That's exactly what I am doing in 20261-5.diff

jkudish23 months ago

remove left-over console.log()

jkudish23 months ago

use $current_checkbox for $current_table selector

jkudish23 months ago

remove svn:ignore that was accidentally included in the previous patch

comment:13 jkudish23 months ago

would love to see this in 3.5-early :)

comment:14 jkudish22 months ago

  • Keywords 3.5-early added; 2nd-opinion removed

SergeyBiryukov22 months ago

comment:15 follow-up: SergeyBiryukov22 months ago

  • Keywords 3.5-early removed
  • Milestone changed from Awaiting Review to 3.5

20261-9.diff is an attempt to simplify the patch.

Only visible checkboxes are taken into account (related: #8355, #8520).

comment:16 in reply to: ↑ 15 jkudish22 months ago

20261-9.diff looks good to me

comment:17 markjaquith22 months ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In [21209]:

Uncheck the "select all" checkbox when one of the child checkboxes is manually unchecked. props SergeyBiryukov. fixes #20261

Note: See TracTickets for help on using tickets.