WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#23543 closed enhancement (fixed)

Twenty Thirteen: Add image navigation to attachment pages

Reported by: obenland Owned by: lancewillett
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

Due to the standard header image we should add an image navigation enhancement similar to what Twenty Eleven had (add an anchor hash to navigation links). Currently it can be pretty annoying to navigate through a gallery on attachment pages with a screen that is less than 900px high.

Attachments (5)

23543.diff (796 bytes) - added by DrewAPicture 3 years ago.
23543.1.diff (1020 bytes) - added by obenland 3 years ago.
navbar-cutting-off-title.png (186.3 KB) - added by kwight 3 years ago.
logged-in.png (235.4 KB) - added by kwight 3 years ago.
logged-out.png (195.1 KB) - added by kwight 3 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 @lancewillett3 years ago

+1, nice usability enhancement.

@DrewAPicture3 years ago

comment:2 @DrewAPicture3 years ago

  • Cc xoodrew@… added
  • Keywords has-patch added; needs-patch removed

23543.diff appends the #main anchor to the attachment links.

I chose #main because it's the latest-running div where the next/previous links aren't hidden below the fixed navbar.

And, because it took me several minutes to work my way through the chain, I'll explain the filter.

  1. next/previous_image_link() calls adjacent_image_link().
  2. adjacent_image_link() retrieves the formatted link via wp_get_attachment_link().
  3. wp_get_attachment_link() calls get_attachment_link() to retrieve the actual URL.
  4. We filter the attachment link URL via the attachment_link filter in get_attachment_link().

comment:3 follow-up: @obenland3 years ago

Nice! I wonder if it would make sense to add the checks that Toolbox has going. What do you think?

comment:4 in reply to: ↑ 3 @DrewAPicture3 years ago

Replying to obenland:

Nice! I wonder if it would make sense to add the checks that Toolbox has going. What do you think?

That's an interesting take on it.

I don't think we to do the extra checks though because adjacent_image_link() only pulls attachments with the mime-type of 'image' anyway.

@obenland3 years ago

comment:5 @obenland3 years ago

  • Keywords needs-testing added

23543.1.diff​ is based off of DrewAPictures work: Moved the function before customizer code to maintain code flow, adjusted comments to Twenty Thirteen style and added a check for images only. We don't really need the anchor on other attachment pages.

The other checks in Toolbox were there to prevent 404s on unattached images when not using pretty permalinks. I was unable to reproduce that in trunk.

comment:6 @kwight3 years ago

Patch works for me. #main makes sense for the anchor, but the title gets cut off by the fixed navbar.

comment:7 @lancewillett3 years ago

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

In 23523:

Twenty Thirteen: usability improvement for better viewing of images on attachment pages. Props DrewAPicture and obenland, fixes #23543.

comment:8 @lancewillett3 years ago

Note: I wasn't able to repeat the cut-off navbar.

@kwight3 years ago

@kwight3 years ago

comment:9 follow-up: @kwight3 years ago

Ah, it has to do with the admin bar (working from r23533).

When logged in, #main slides up behind both the admin bar and the fixed navbar, resulting in the title and post meta being covered up (except for an ugly artifact on the right from bottom of the meta).

When not logged in, #main only slides up behind the fixed navbar, leaving the title and meta showing but cut off.

I admit neither looks good to me. For fun I even tried the higher up #navbar as the anchor, but that just caused page jumping because of the fixed navbar.

Possible solutions? Maybe a JS hack to add top margin when the bars are present (ick). Of course, with no fixed navbar, everything is perfect :)

comment:10 in reply to: ↑ 9 @DrewAPicture3 years ago

Replying to kwight:

When not logged in, #main only slides up behind the fixed navbar, leaving the title and meta showing but cut off.

I admit neither looks good to me. For fun I even tried the higher up #navbar as the anchor, but that just caused page jumping because of the fixed navbar.

I suppose we could insert a special jump point where we needed it, but that feels like a band-aid.

comment:11 @lancewillett2 years ago

  • Keywords needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

See also #23719, a bug that surfaced and results in this improvement being reverted.

comment:12 @lancewillett2 years ago

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

In 23677:

Twenty Thirteen: revert addition of twentythirteen_attachment_link() in r23523 -- as discussed in the following ticket comments, the tradeoff means simpler code instead of a minor UX enhancement. Closes #23543 and #23719.

Note: See TracTickets for help on using tickets.