WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#27533 closed enhancement (fixed)

Unobtrusive JS for "Quick Edit" button at "Comments" page and others

Reported by: aubreypwd Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.9
Component: Administration Keywords: has-patch commit dev-reviewed
Focuses: javascript, administration Cc:

Description

In #24741 I fixed the issue where onclick= was being used to call a function, but it wasn't the ideal fix. We want to fix that instance, and other applicable instances in core (if any), to be unobtrusive if possible.

Attachments (6)

27533.diff (2.8 KB) - added by aubreypwd 7 years ago.
Using unobtrusive JS on comment "Reply," and "Quick Edit."
27533.2.diff (2.9 KB) - added by aubreypwd 7 years ago.
Update per @wonderboymusic suggestion with delegation.
27533.3.diff (2.9 KB) - added by aubreypwd 7 years ago.
Updates to tabs.
27533.4.diff (1.4 KB) - added by helen 6 years ago.
27533.5.diff (2.4 KB) - added by helen 6 years ago.
27533.6.diff (2.7 KB) - added by helen 6 years ago.

Download all attachments as: .zip

Change History (20)

#1 @SergeyBiryukov
7 years ago

  • Component changed from General to Administration

@aubreypwd
7 years ago

Using unobtrusive JS on comment "Reply," and "Quick Edit."

#2 @aubreypwd
7 years ago

  • Keywords has-patch dev-feedback added

I've added a patch to solve #24741 using un-obtrusive JS.

This ticket was mentioned in IRC in #wordpress-dev by aubreypwd. View the logs.


7 years ago

#4 @aubreypwd
7 years ago

@SergeyBiryukov I saw you slated another ticket of mine for 4.0, can we get this one in? I think it's better than what's there.

This ticket was mentioned in IRC in #wordpress-dev by aubreypwd. View the logs.


7 years ago

#6 @wonderboymusic
7 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Milestone changed from Awaiting Review to 4.0

I like this but it should use event delegation:

$( 'body' ).on( 'click', '.class-name, .other-class', function (e) {
    e.preventDefault();

    var element = $( e.currentTarget );

   // whatever 
}  );

@aubreypwd
7 years ago

Update per @wonderboymusic suggestion with delegation.

#7 @aubreypwd
7 years ago

  • Keywords has-patch added; needs-patch removed

@aubreypwd
7 years ago

Updates to tabs.

#8 @wonderboymusic
7 years ago

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

In 28810:

Use unobtrusive JS for Comment list table row actions.

Props aubreypwd.
Fixes #27533.

#9 @nacin
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[28810] does a few particularly weird things:

  • It lets things bubble up to the body, rather than using a more direct parent like #the-comment-list or table.comments or whatever.
  • It uses a lot of unnecessary cascading: .comment .row-actions .quickedit .edit-comment-inline. Something like .edit-comment-inline should be sufficient.
  • Why does it use event.currentTarget rather than this?
  • commentReply has to be defined; it's in this JS file. Those checks were only there because it was obtrusive JS and it could have been triggered before this file was loaded.

@helen
6 years ago

#10 @helen
6 years ago

27533.4.diff addresses the above points, all of which I agree with. In particular, I would love to know why there's been a proliferation of event.currentTarget instead of this recently, and if there are any specific places that actually need the differentiation.

@helen
6 years ago

#11 @helen
6 years ago

27533.5.diff consolidates the callback.

@helen
6 years ago

#12 @nacin
6 years ago

  • Keywords commit dev-reviewed added

#13 @helen
6 years ago

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

In 29662:

Comments: Simplify JS for inline edit and reply.

fixes #27533.

#14 @aubreypwd
6 years ago

My original patch used this, but I took @wonderboymusic's advice on a second patch; that was all.

Note: See TracTickets for help on using tickets.