Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#15580 closed defect (bug) (fixed)

Fixes to Ajaxify Admin before 3.1

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

Download all attachments as: .zip

Change History (111)

#1 @nacin
13 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.

#2 @nacin
13 years ago

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

#3 @jane
13 years ago

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

#4 @westi
13 years ago

  • Cc westi added

Also #15500

#5 @scribu
13 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.

#6 @scribu
13 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.

#7 @scribu
13 years ago

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

#8 @scribu
13 years ago

  • Description modified (diff)

#9 follow-up: @scribu
13 years ago

Date columns should first sort by DESC, not ASC.

Why?

#10 in reply to: ↑ 9 @nacin
13 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.

#11 @scribu
13 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.

#12 @scribu
13 years ago

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

#13 follow-up: @scribu
13 years ago

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

#14 @scribu
13 years ago

  • Description modified (diff)

#15 in reply to: ↑ 13 @nacin
13 years ago

Replying to scribu:

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

Great work. :-)

#16 @scribu
13 years ago

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

@scribu
13 years ago

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

#17 @voyagerfan5761
13 years ago

  • Cc WordPress@… added

#18 @scribu
13 years ago

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

#19 @scribu
13 years ago

  • Description modified (diff)

#20 @ryan
13 years ago

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

@scribu
13 years ago

#21 @scribu
13 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.

#22 @ryan
13 years ago

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

#23 @scribu
13 years ago

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

#24 @ryan
13 years ago

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

#25 @scribu
13 years ago

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

#26 @ryan
13 years ago

Yeah, popState support is pretty young.

#27 @ryan
13 years ago

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

#28 @scribu
13 years ago

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

#29 @scribu
13 years ago

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

#30 @scribu
13 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.

#31 @scribu
13 years ago

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

#32 @scribu
13 years ago

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

#33 @scribu
13 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.

#34 @nacin
13 years ago

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

#35 @scribu
13 years ago

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

#36 follow-up: @scribu
13 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.

#37 @nacin
13 years ago

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

#38 in reply to: ↑ 36 @mikeschinkel
13 years ago

Replying to scribu:

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

+1

#39 @scribu
13 years ago

  • Keywords ui-feedback added

#40 @mikeschinkel
13 years ago

  • Cc mikeschinkel@… added

#41 @scribu
13 years ago

Aftermath: #15609

#42 follow-up: @nacin
13 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.

#43 @scribu
13 years ago

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

#44 @scribu
13 years ago

  • Description modified (diff)

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

#45 @batmoo
13 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.

#46 @scribu
13 years ago

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

#47 @lloydbudd
13 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.

#48 @JustinSainton
13 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.

#49 @JustinSainton
13 years ago

I'm a tool.

manage_edit-$ptype_sortable_columns

@batmoo
13 years ago

Clean up loading overlay

#50 in reply to: ↑ 42 @batmoo
13 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 :)

#51 follow-up: @scribu
13 years ago

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

#52 @scribu
13 years ago

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

@batmoo
13 years ago

use .html() instead of .append()

@batmoo
13 years ago

Large loady spinner

#53 in reply to: ↑ 51 @batmoo
13 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.

@scribu
13 years ago

problems with large spinner

#54 follow-up: @scribu
13 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)

@batmoo
13 years ago

Fixes sizing issues, alignment, and scrolling

#55 in reply to: ↑ 54 @batmoo
13 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).

#56 follow-up: @scribu
13 years ago

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

#57 @scribu
13 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.

#58 in reply to: ↑ 56 @westi
13 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

#59 @westi
13 years ago

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

#60 @westi
13 years ago

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

#61 @JohnONolan
13 years ago

  • Keywords ux-feedback added

Screenshots are always great if possible

@scribu
13 years ago

Posts screen with spinner

@scribu
13 years ago

Comments screen with spinner

#63 @garyc40
13 years ago

  • Cc garyc40@… added

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

see #15416 for patch

#64 @scribu
13 years ago

  • Description modified (diff)

#65 @scribu
13 years ago

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

#66 @JohnONolan
13 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?

#67 follow-up: @scribu
13 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.

#68 @JohnONolan
13 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.

#69 in reply to: ↑ 67 @batmoo
13 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.

#70 follow-up: @JohnONolan
13 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

#71 in reply to: ↑ 70 @batmoo
13 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 :)

#72 @scribu
13 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)

#73 @nacin
13 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.

@scribu
13 years ago

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

#74 @scribu
13 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

@scribu
13 years ago

add bottom spinner

@scribu
13 years ago

add spinners to plugin install list table

@scribu
13 years ago

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

#75 @scribu
13 years ago

  • Keywords commit added

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

#76 @ryan
13 years ago

Looks good.

#77 @ryan
13 years ago

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

@scribu
13 years ago

CSS tweaks

#78 @scribu
13 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

@scribu
13 years ago

for padding.15580.diff

@scribu
13 years ago

for padding.15580.diff

#79 @nacin
13 years ago

Can we make the sorting arrows a sprite?

#80 @JohnONolan
13 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

#81 @scribu
13 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.

#82 @nacin
13 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.

#83 @scribu
13 years ago

  • Keywords commit removed

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

#84 @dd32
13 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)

#85 @scribu
13 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. :)

@scribu
13 years ago

@scribu
13 years ago

@scribu
13 years ago

Use sprite

#86 @scribu
13 years ago

  • Keywords commit added

#87 @ryan
13 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

#88 @hakre
13 years ago

Related: #16022

#89 @nacin
13 years ago

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

#90 @hakre
13 years ago

Related: #16162; #16163; #16166

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

#91 @hakre
13 years ago

Related: #16185

Note: See TracTickets for help on using tickets.