WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#29286 closed enhancement (wontfix)

Plugin cards: make whole card open info modal

Reported by: stephdau Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.0
Component: Plugins Keywords: has-patch needs-testing 2nd-opinion
Focuses: Cc:

Description

Attached is a patch to make clicking anywhere in a plugin card (search results, see #28785) open the plugin info modal.

It specifically avoids links not meant to open the modal, such as the install button and link to the developers' site.

Attachments (6)

29286.diff (1.0 KB) - added by stephdau 3 years ago.
29286.2.diff (1.4 KB) - added by stephdau 3 years ago.
Same as 29286.diff, but also changes the mouse cursor to the pointer one when over a card.
29286.3.diff (1.5 KB) - added by stephdau 3 years ago.
29286.4.diff (1.7 KB) - added by stephdau 3 years ago.
29286.5.diff (1.7 KB) - added by stephdau 3 years ago.
29286.6.diff (1.7 KB) - added by aaroncampbell 3 years ago.

Download all attachments as: .zip

Change History (19)

@stephdau
3 years ago

#1 @stephdau
3 years ago

  • Keywords has-patch added

#2 @stephdau
3 years ago

  • Keywords needs-testing added

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


3 years ago

@stephdau
3 years ago

Same as 29286.diff, but also changes the mouse cursor to the pointer one when over a card.

#4 @nacin
3 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to 4.0

I'm a nervous clicker so helen asked me to take a look at this. I was playing around with this earlier today and I don't dislike it... But it also means I can't select anything. Bailing if document.getSelection().type === 'Range' makes this feel a lot better and then I'm a bit more inclined to go with it.

Moving to 4.0 for review. Could use additional opinions.

@stephdau
3 years ago

#5 @stephdau
3 years ago

attachment:29286.3.diff adds a test for document.getSelection().type === 'Range' to not trigger thickbox when a user is selecting some text within the card. Props Nacin (didn't know that one, great stuff).

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


3 years ago

@stephdau
3 years ago

@stephdau
3 years ago

#7 @stephdau
3 years ago

attachment:29286.5.diff adds multiple tests for getting a selection object in diff browsers (props ocean90) so it works in older MSIE.

Last edited 3 years ago by stephdau (previous) (diff)

#8 @ocean90
3 years ago

29286.5.diff:

  • document.selection is no function in IE, so document.selection() produces an error
  • document.selection.type is text in IE 8
  • Opera 12, Firefox 31 and IE > 9 doesn't support the type property
  • We can remove document.getSelection() in favour of just window.getSelection(), the later on seems to be the right one

#9 @aaroncampbell
3 years ago

29286.6.diff is just a refresh of .5 that applies cleanly.

The patch works fine for me in Chrome, but not in Firefox. In Firefox it looks like s.type doesn't exist, so selecting a range still opens a modal.

#10 @aaroncampbell
3 years ago

The only obvious way that I see (and I make no guarantees I'm not missing something here) is to use var r = s.getRangeAt(0) and then compare r.startOffset and r.endOffset

This works in Chrome and Firefox, but I don't have IE anywhere here to test.

#11 @aaroncampbell
3 years ago

Obviously I'm coming to this a little late, but I'm wondering what the impetus for this was in the first place. I'm with Nacin that not being able to select text in the card would be annoying to me. Now as I look at it, the code to be able to properly handle checking selection seems excessive for the gains, but maybe I'm not fully understanding the gains here. It looks like the "more details" link, the plugin title, and the icon all already open the modal, which seems like plenty for me.

#12 @nacin
3 years ago

Now that a giant icon tap target is there, I do wonder if we should bother with this. (There are also probably more pressing things at this point.)

#13 @helen
3 years ago

  • Milestone 4.0 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Agreed that this is a not-insignificant amount of effort and code for something that doesn't seem to be a huge benefit.

Note: See TracTickets for help on using tickets.