WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15580 closed defect (bug) (fixed)

Fixes to Ajaxify Admin before 3.1

Reported by: jane Owned by: scribu
Milestone: 3.1 Priority: highest omg bbq
Severity: blocker Version: 3.1
Component: Administration Keywords: has-patch commit
Focuses: Cc:

Description (last modified by scribu)

This ticket will hold the individual adjustments needed for the ajaxified admin features before we release for 3.1

  1. When using pagination located at the bottom of the table, should load new results and bump you to the top of the screen.
  1. On comments, no need to sort alphabetically by comment.
  1. On p=36220 comment screens, no need to sort by "in reply to" since they are all in reply to the same comment.
  1. Save-state hashes for URLs.

4b. Don't bump bulk action and other triggers back to the front page.

  1. Date columns should first sort by DESC, not ASC. Plugins will likely need a new way to request DESC over ASC for a specific column.

Attachments (20)

comments.diff (1.6 KB) - added by scribu 3 years ago.
Removes 'In Reply To' column. Doesn't work for inline editing & replying
hashes.diff (11.9 KB) - added by scribu 3 years ago.
15580_loading_overlay.diff (1.8 KB) - added by batmoo 3 years ago.
Clean up loading overlay
15580_loading_overlay_v2.diff (1.9 KB) - added by batmoo 3 years ago.
use .html() instead of .append()
loading-lg.gif (3.1 KB) - added by batmoo 3 years ago.
Large loady spinner
Screenshot.png (41.2 KB) - added by scribu 3 years ago.
problems with large spinner
15580_loading_overlay_v3.diff (2.6 KB) - added by batmoo 3 years ago.
Fixes sizing issues, alignment, and scrolling
screenshot-1.png (88.9 KB) - added by scribu 3 years ago.
Posts screen with spinner
screenshot-2.png (78.4 KB) - added by scribu 3 years ago.
Comments screen with spinner
spinner-search.png (93.6 KB) - added by scribu 3 years ago.
Replace 'Loading...' and overlay with small spinner to the left of the search box
spinner-search.diff (3.6 KB) - added by scribu 3 years ago.
spinner-search.2.diff (3.8 KB) - added by scribu 3 years ago.
add bottom spinner
spinner-search.3.diff (4.6 KB) - added by scribu 3 years ago.
add spinners to plugin install list table
spinner-search.4.diff (6.7 KB) - added by scribu 3 years ago.
add spinners to themes, theme-install and plugin-install
padding.15580.diff (1.1 KB) - added by scribu 3 years ago.
CSS tweaks
sort_asc.gif (48 bytes) - added by scribu 3 years ago.
for padding.15580.diff
sort_desc.gif (49 bytes) - added by scribu 3 years ago.
for padding.15580.diff
sort.psd (628 bytes) - added by scribu 3 years ago.
sort.gif (54 bytes) - added by scribu 3 years ago.
padding.15580.2.diff (1.2 KB) - added by scribu 3 years ago.
Use sprite

Download all attachments as: .zip

Change History (111)

comment:1 nacin3 years ago

  • Description modified (diff)
  • Keywords needs-patch added; ajax removed
  • Milestone changed from Awaiting Review to 3.1
  • Severity changed from normal to blocker

Adding a few more issues.

comment:2 nacin3 years ago

  • Priority changed from normal to highest omg bbq
  • Status changed from new to assigned

comment:3 jane3 years ago

Change default sort by chronology to be reversed by default (most recent first).

comment:4 westi3 years ago

  • Cc westi added

Also #15500

comment:5 scribu3 years ago

  1. On p=36220 comment screens, no need to sort by "in reply to" since they are all in reply to the same comment.

For clarity, the column is called "In response to" and I think it should simply not be shown at all.

comment:6 scribu3 years ago

It would be great if someone could take a look at http://core.trac.wordpress.org/ticket/14579#comment:158 as I don't have access to Internet Explorer.

comment:7 scribu3 years ago

(In [16588]) Make comment content column unsortable. See #15580

comment:8 scribu3 years ago

  • Description modified (diff)

comment:9 follow-up: scribu3 years ago

Date columns should first sort by DESC, not ASC.

Why?

comment:10 in reply to: ↑ 9 nacin3 years ago

Replying to scribu:

Date columns should first sort by DESC, not ASC.

Why?

Per Jane:

Change default sort by chronology to be reversed by default (most recent first).

This is the expected functionality. Oldest first is unintuitive.

comment:11 scribu3 years ago

Oldest first is unintuitive only if you're sorting by date, but there's only one date column.

When you're sorting by title, you don't expect to see the post with title Z first.

comment:12 scribu3 years ago

Ah, I now saw that it's only supposed to be applied for date columns. Sorry.

comment:13 follow-up: scribu3 years ago

(In [16593]) Make date columns first sort in descending order. See #15580

comment:14 scribu3 years ago

  • Description modified (diff)

comment:15 in reply to: ↑ 13 nacin3 years ago

Replying to scribu:

(In [16593]) Make date columns first sort in descending order. See #15580

Great work. :-)

comment:16 scribu3 years ago

(In [16594]) Update get_*_columns() method docs. See #15580

scribu3 years ago

Removes 'In Reply To' column. Doesn't work for inline editing & replying

comment:17 voyagerfan57613 years ago

  • Cc WordPress@… added

comment:18 scribu3 years ago

(In [16602]) Make bottom admin pagination scroll to the top after rows are updated. See #15580

comment:19 scribu3 years ago

  • Description modified (diff)

comment:20 ryan3 years ago

What are the plans for hashes? Use one of the many jQuery plugins? Handroll?

scribu3 years ago

comment:21 scribu3 years ago

For hashes, I began rolling out a custom solution because I didn't really find any jQuery plugins that would help. jquery.query.js seems to be the only thing needed.

hashes.diff:

  • changing the rows changes the hash.
  • going to an url with a hash changes the rows. But, it doesn't change the ajaxified elements, like the page number etc.

comment:22 ryan3 years ago

Crazy notion: Support pushState()/popState() and forget hashes.

comment:23 scribu3 years ago

I thought the main reason for using hashes was to support bookmarking, not history.

comment:24 ryan3 years ago

Don't those update the URL in the location bar so that a proper link is shown?

comment:25 scribu3 years ago

That would be awesome. However, it's not supported in any current browser yet.

comment:26 ryan3 years ago

Yeah, popState support is pretty young.

comment:27 ryan3 years ago

Seems to work in Chrome and FF4 will support it. Very tempting to go modern, especially given what a hack hashes are. :-)

comment:28 scribu3 years ago

I'll play around with it in Chrome, then.

comment:29 scribu3 years ago

(In [16615]) Update URL when performing ajaxified action. See #15580

comment:30 scribu3 years ago

With pushState(), when the user presses the back button, only the URL is updated, so - just like with hashes, we would have to take care of updating the rows and the UI elements again.

So, I used replaceState() instead, which doesn't create a new history entry.

comment:31 scribu3 years ago

(In [16616]) Add missing quote char when creating overlay. See #14579. See #15580

comment:32 scribu3 years ago

(In [16617]) Get rid of in favor of . Fixes #15607. See #15580

comment:33 scribu3 years ago

Yeah, the commit message was supposed to read "Get rid of $mode in favor of $post_id".

$mode seems to be a relic (it could be either 'single' or 'detail'), possibly since to edit a single comment you used edit-comments.php too?

Anyway, I stopped using it in favor of just checking the $post_id.

comment:34 nacin3 years ago

The $mode global also applies to edit.php, how you have the excerpt view.

comment:35 scribu3 years ago

Sure, but I only stopped using it in the comments screen, because it had no effect there.

comment:36 follow-up: scribu3 years ago

From http://core.trac.wordpress.org/ticket/14579#comment:164:

I've noticed that when you sort by title in a list of hierarchical posts you loose the hierarchy indicators (which is what I would expect) but there seems to be no way to go back to the hierarchical listing without refreshing the entire page. Not having some way to revert could be confusing for people and while a page refresh will fix, that being the only way feels like it was just missed in the design process.

I considered a page refresh a good-enough method. However, that won't work since r16615: the sorting order will remain unchanged.

So, the only way to go back to the hierarchical order now is by clicking on the admin menu link again.

Under this circumstance, I think a UI element for going back to the hierarchical order would be useful.

comment:37 nacin3 years ago

A page refresh will also no longer clear search results: #15355.

comment:38 in reply to: ↑ 36 mikeschinkel3 years ago

Replying to scribu:

Under this circumstance, I think a UI element for going back to the hierarchical order would be useful.

+1

comment:39 scribu3 years ago

  • Keywords ui-feedback added

comment:40 mikeschinkel3 years ago

  • Cc mikeschinkel@… added

comment:41 scribu3 years ago

Aftermath: #15609

comment:42 follow-up: nacin3 years ago

Strongly considering the need for an AJAX spinner. (See #15482.) Otherwise it seems like you get no real feedback on pagination, sorting, or searching.

comment:43 scribu3 years ago

  • Description modified (diff)
  • Keywords needs-ui added; ui-feedback removed

comment:44 scribu3 years ago

  • Description modified (diff)

Really have no idea on what to do about 4b.

comment:45 batmoo3 years ago

If I enter non-digits (e.g. 'abc') into the page number input and search, it returns an NaN. We can either have the parseInt call (in the keypress function for .current-page) fallback to 1

var page = parseInt( $el.val() ) || 1;
change_page( page, $el );

or alert the user and ask them to enter a valid number.

Similarly, if a user enters a page number higher than the total pages or below 1, we should either handle it gracefully (entering 11 with 9 total pages sends you to page 9; entering -1 sends you to page 1) or alert them.

comment:46 scribu3 years ago

(In [16644]) Validate pagination input. Props batmoo. See #15580

comment:47 lloydbudd3 years ago

4b. Don't bump bulk action and other triggers back to the front page.

ENV: wp trunk r16653 ( 3.1-beta1-16642 )

I take it this describes the scenario where the search term is cleared after a bulk action.

comment:48 JustinSainton3 years ago

  • Cc JustinSainton added

Doesn't appear to work on custom post type pages when custom columns are added.
http://scribu.net/wordpress/custom-sortable-columns.html shows custom columns working on regular post types, and it appears to work fine for non-custom columns on custom post types...just not custom columns on custom types.

comment:49 JustinSainton3 years ago

I'm a tool.

manage_edit-$ptype_sortable_columns

batmoo3 years ago

Clean up loading overlay

comment:50 in reply to: ↑ 42 batmoo3 years ago

  • Cc batmoo@… added

Replying to nacin:

Strongly considering the need for an AJAX spinner. (See #15482.) Otherwise it seems like you get no real feedback on pagination, sorting, or searching.

Patch (15580_loading_overlay.diff) cleans up the loading overlay. I've added a large version of the loading spinner since the small ones included with WP would be easily missed. Works nicely in IE6 too :)

comment:51 follow-up: scribu3 years ago

Is there a specific reason for using .append() instead of .html() ?

comment:52 scribu3 years ago

Also, please upload the spinner graphic separately (it's not contained in the patch).

batmoo3 years ago

use .html() instead of .append()

batmoo3 years ago

Large loady spinner

comment:53 in reply to: ↑ 51 batmoo3 years ago

Replying to scribu:

Is there a specific reason for using .append() instead of .html() ?

Ah, yeah, probably better to use .html(). Not sure why I went with .append()

Replying to scribu:

Also, please upload the spinner graphic separately (it's not contained in the patch).

Uploaded.

scribu3 years ago

problems with large spinner

comment:54 follow-up: scribu3 years ago

A couple of problems with the large spinner:

  • the overlay is about 10px higher than the rows
  • the spinner isn't centered vertically, which looks weird when there's only a single row.

Both problems can be observed in Screenshot.png (made using Firefox 3.6)

batmoo3 years ago

Fixes sizing issues, alignment, and scrolling

comment:55 in reply to: ↑ 54 batmoo3 years ago

Replying to scribu:

  • the overlay is about 10px higher than the rows
  • the spinner isn't centered vertically, which looks weird when there's only a single row.

Overlay size should be fixed in new patch. I've moved the spinner very close to the top so it should look and work well on both single and multiple rows. I've also moved the scrollTo right before the overlay is displayed; this is so that you see the loading feedback any time you trigger an action (particularly way down at the bottom of the page, such as the bottom pagination).

comment:56 follow-up: scribu3 years ago

(In [16719]) Use large spinner instead of 'Loading...' text. Props batmoo. See #15580

comment:57 scribu3 years ago

I have to admin I'm not crazy about the grayish background of the overlay, but it does provide better contrast against the comments list for example, which is all white.

comment:58 in reply to: ↑ 56 westi3 years ago

Replying to scribu:

(In [16719]) Use large spinner instead of 'Loading...' text. Props batmoo. See #15580

Changes like this especially post beta 1 should wait for UI feedback from Jane before commit.

Reverting for now while we discuss this

comment:59 westi3 years ago

(In [16722]) Revert [16719] until it has has UI review. See #15580

comment:60 westi3 years ago

I'm not going to bother rebumping the script vers after this as it hasn't made it into a nightly build

comment:61 JohnONolan3 years ago

  • Keywords ux-feedback added

Screenshots are always great if possible

scribu3 years ago

Posts screen with spinner

scribu3 years ago

Comments screen with spinner

comment:63 garyc403 years ago

  • Cc garyc40@… added

"4b. Don't bump bulk action and other triggers back to the front page."

see #15416 for patch

comment:64 scribu3 years ago

  • Description modified (diff)

comment:65 scribu3 years ago

  • Keywords has-patch added; needs-patch needs-ui removed

comment:66 JohnONolan3 years ago

What is the orientation of the spinner based on? As in - is there any particular reason for it to be right at the top in the middle? I'm guessing you're thinking about small screen sizes?

comment:67 follow-up: scribu3 years ago

The top-middle is most likely to be seen.

Didn't think about the case where the user is at the footer of the table.

comment:68 JohnONolan3 years ago

Would it make more sense to put a standard spinner beside both of the "filter" buttons whenever they're clicked? A bit like the save-draft button when writing a new post.

comment:69 in reply to: ↑ 67 batmoo3 years ago

Replying to scribu:

Didn't think about the case where the user is at the footer of the table.

The patch jumps the user to the top of the page any time the table content is changed.

Replying to JohnONolan:

Would it make more sense to put a standard spinner beside both of the "filter" buttons whenever they're clicked? A bit like the save-draft button when writing a new post.

That's what I was originally thinking with #15482, but if we go that route, we'll end up with spinners everywhere. We'd need separate ones for pagination, filter, sorting, and so on. Or, we go with a single one in a prominent location.

comment:70 follow-up: JohnONolan3 years ago

I'm not sure that jumping to the top of the page is great UX for an AJAX filter...?

Spinners beside actions is expected functionality at the moment, I'm pretty sure that's how almost everything works. This would be a move away from that and establishing a new type of feedback to an interaction. That's not to say it isn't a good idea - it just needs to be considered carefully in terms of both approach and implications.

Keyword: needs-jane

comment:71 in reply to: ↑ 70 batmoo3 years ago

Replying to JohnONolan:

I'm not sure that jumping to the top of the page is great UX for an AJAX filter...?

It makes sense when the user is at the bottom of the page and takes action. The reason my patch always jumps the user up is to make the loading indicator visible, otherwise it can feel like nothing happened.

Currently, only the bottom pagination jumps you up, and that probably make sense. Though, Bulk Edit should jump you up too.

Replying to JohnONolan:

Spinners beside actions is expected functionality at the moment, I'm pretty sure that's how almost everything works. This would be a move away from that and establishing a new type of feedback to an interaction. That's not to say it isn't a good idea - it just needs to be considered carefully in terms of both approach and implications.

Agreed, consistency is awesome. And I like the WordPress standard spinner :)

comment:72 scribu3 years ago

  • Keywords needs-patch added; has-patch ux-feedback removed

UI changes to be made, per IRC chat today:

  • drop the overlay
  • paging: small spinner next to the pagination
  • search: small spinner next to search
  • sorting: sorting arrow transforms into spinner

(at least that's what I understood)

comment:73 nacin3 years ago

I understood just two spinners, next to each pagination row, to be triggered on all actions. (There might be more room for them to appear to the left of the search box, even.)

Spinners everywhere is perhaps a long-term goal, but that's more cumbersome to do and implement for 3.1 I think.

scribu3 years ago

Replace 'Loading...' and overlay with small spinner to the left of the search box

scribu3 years ago

comment:74 scribu3 years ago

  • Keywords has-patch added; needs-patch removed

spinner-search.diff adds a small spinner next to the search box. The pagination row is too crowded.

See spinner-search.png

scribu3 years ago

add bottom spinner

scribu3 years ago

add spinners to plugin install list table

scribu3 years ago

add spinners to themes, theme-install and plugin-install

comment:75 scribu3 years ago

  • Keywords commit added

I think spinner-search.4.diff covers all the screens.

comment:76 ryan3 years ago

Looks good.

comment:77 ryan3 years ago

(In [17090]) List table spinner. Props scribu. see #15580

scribu3 years ago

CSS tweaks

comment:78 scribu3 years ago

padding.15580.diff:

  • makes 100% of the <th> clickable
  • vertically aligns the sorting indicator on the comments column with the rest
  • removes useless padding in the sorting indicator images

scribu3 years ago

for padding.15580.diff

scribu3 years ago

for padding.15580.diff

comment:79 nacin3 years ago

Can we make the sorting arrows a sprite?

comment:80 JohnONolan3 years ago

If you do make a sprite pls upload a copy of the PSD (or other graphics file) to the ticket so that we can archive it for future use

comment:81 scribu3 years ago

I will make a sprite.

I think a PSD would be a major overkill in this case.

Previous versions of files are automatically kept in the SVN revisions.

comment:82 nacin3 years ago

We're working on an SVN repo for artwork. That will include originals for every core file.

Original files need to be uploaded from now on. Otherwise there won't be a commit. We're done with rebuilding images from scratch every time we need to modify them.

comment:83 scribu3 years ago

  • Keywords commit removed

Ok, cool. Well, the original images are already uploaded: sort_asc.gif and sort_desc.gif

comment:84 dd323 years ago

the original files which the arrows come from would be appreciated as well, ie. modifying gif's is impossible, better to give a high-res psd or equiv. which modifications can be based off for future changes (ie. colour changes in those gifs would be impossible, but re-building from a decent res psd would be easy)

comment:85 scribu3 years ago

The arrows were hand-drawn pixel by pixel, so I don't have any larger-res files.

I hope it is clear by now that my Photoshop skills are... limited. :)

scribu3 years ago

scribu3 years ago

scribu3 years ago

Use sprite

comment:86 scribu3 years ago

  • Keywords commit added

comment:87 ryan3 years ago

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

(In [17110]) Put sort arrows in sprite. Make 100% of th clickable. Align sort indicators. Remove unneeded padding. Props scribu. fixes #15580

comment:88 hakre3 years ago

Related: #16022

comment:89 nacin3 years ago

(In [17195]) div.alignleft.actions needs to wrap the 'Empty Trash' button too, otherwise they're not aligned. see #15580.

comment:90 hakre3 years ago

Related: #16162; #16163; #16166

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

comment:91 hakre3 years ago

Related: #16185

Note: See TracTickets for help on using tickets.