WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#32711 closed enhancement (wontfix)

Customizer Menus: link items should have view links also

Reported by: designsimply Owned by: westonruter
Milestone: Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch 2nd-opinion
Focuses: ui, javascript Cc:

Description

Moving this over from https://github.com/voldemortensen/menu-customizer/issues/104

From @celloexpressions:

We have handy "original" links for items that represent post or taxonomy term objects. We should add similar functionality for custom links and for bonus points, make it open in the Customizer preview if it's a previewable URL or otherwise open it in a new tab. Currently your best workflows are to copy/paste the URL or click on the link in the preview (which wouldn't work if it is an external link).

From @westonruter:

Note that original links for external custom links won't work in the Customizer either, since the Customizer preview will reject previewing any URL that is not under the site.

Semi-related: #32681

Attachments (2)

32711.diff (1.4 KB) - added by celloexpressions 6 years ago.
Allow custom links to be previewed, or opened in a new tab if they aren't previewable.
32711.2.diff (2.6 KB) - added by celloexpressions 6 years ago.
Implement @westonruter's suggestions.

Download all attachments as: .zip

Change History (12)

@celloexpressions
6 years ago

Allow custom links to be previewed, or opened in a new tab if they aren't previewable.

#1 @celloexpressions
6 years ago

  • Focuses ui javascript added
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.3
  • Version set to trunk

This is pretty straightforward - we can just let the link open normally (in a new tab, out of necessity) if it isn't previewable. The only thing I'm having trouble with in the patch is that ideally we could get back the result of the attempt to preview this URL here and use it as a conditional for calling e.preventDefault(). Not sure what the best way to do this is, anyone have thoughts?

Should also probably add live-updating of the URL here when the URL field is changed (not that it would be frequently). Having a preview link is actually pretty important - this functionality is typically present in other places where you add a link by URL so that you can check it before publishing. We can't assume that users know how to open links in new tabs themselves, or that they know they can do that in the Customizer preview if a link isn't working.

#2 @westonruter
6 years ago

Here's the logic for how the Customizer determines whether or not a URL can be previewed: https://github.com/xwp/wordpress-develop/blob/322b6cc54fcdcd078fe8fefd6e66ebe67dc7de16/src/wp-admin/js/customize-controls.js#L2507-L2543

So what you could do is something like this:

var originalLink = navMenuItemControl.container.find( '.original-link' );

originalLink.attr( 'target', '_blank' );
originalLink.on( 'click', function( e ) {
        var previousPreviewedUrl = wp.customize.previewer.previewUrl.get();
        if ( previousPreviewedUrl === this.href ) {
                // URL is already being previewed, so do nothing.
                e.preventDefault();
                return;
        }
        wp.customize.previewer.previewUrl.set( this.href );
        if ( previousPreviewedUrl !== wp.customize.previewer.previewUrl.get() ) {
                /*
                 * URL can successfully be previewed (and will now be), 
                 * so don't let URL open in new window.
                 */
                e.preventDefault();
                return;
        }
        /*
         * At this point, since the URL in the link is not the same as the currently-previewed
         * URL, and since the setter for wp.customize.previewer.previewUrl rejected the link 
         * URL as not being previewable, we let the browser do the default action and open 
         * the URL in a new window.
         */
} );

// Update the link whenever the menu item's URL changes.
navMenuItemControl.setting.bind( function ( newNavMenuItem, oldNavMenuItem ) {
        var previousPreviewUrl;
        if ( navMenuItem && newNavMenuItem.url !== oldNavMenuItem.url ) {
                originalLink.attr( 'href', navMenuItem.url );
        }
} );

@celloexpressions
6 years ago

Implement @westonruter's suggestions.

#3 @celloexpressions
6 years ago

32711.2.diff implements westonruter's suggestions and should fix this ticket.

#4 @celloexpressions
6 years ago

  • Keywords commit added
  • Owner set to westonruter
  • Status changed from new to reviewing

This should be ready for commit.

#5 @ocean90
6 years ago

  • Keywords 2nd-opinion added; commit removed

Having some links opening in a new tab and some in the preview feels a bit odd. Using "Original" and "View" for more or less the same functionality doesn't make it easier to understand this behaviour.

#6 @celloexpressions
6 years ago

Agree it's not ideal, but it's the best option here. Currently we don't provide any path to viewing link URLs, which is something users would want to do to check that it's typed properly, remind themselves of where it goes, etc. We really need to avoid direct links in the Customizer or other places where the link will navigate you away from the customization flow and your unsaved changes. Opening previable links in the Customizer preview is an awesome enhancement with ideal flow. For other links, we can still provide the most reasonable user flow - opening the link, but not navigating the page away from the Customizer - which is a bit disruptive but allows them to easily get back to the flow they were in.

In terms of view vs. original, for the original links the link text is the original title of the object that the menu item represents (this is taken from the admin screen handling). But "original" doesn't make sense for manual links that aren't tied to WordPress objects, so those get view.

On either of those issues, we shouldn't let these hold back this basic missing functionality in 4.3, so if you feel strongly, feel free to change them, but I disagree. Also, in the future (and this is also my fault for not pushing sooner), let's not let a ticket with a seemingly-finalized patch sit for 10 days without activity until we're cutting it super tight on beta.

This ticket was mentioned in Slack in #core by obenland. View the logs.


6 years ago

#8 follow-up: @westonruter
6 years ago

I just found a bug with the Original link: it is not populated for newly-added menu items.

#9 in reply to: ↑ 8 ; follow-up: @samuelsidler
6 years ago

  • Milestone 4.3 deleted
  • Resolution set to wontfix
  • Status changed from reviewing to closed

Replying to westonruter:

I just found a bug with the Original link: it is not populated for newly-added menu items.

Can you file a new ticket for that? (See also #32814.)

Just to recap this ticket:

Currently, when you add a "Page" to a menu, it includes an "Original" link that lets you view said page. This is true in nav-menus.php and in the Customizer. In the case of the Customizer, the "Original" link opens in the live-preview view.

However, for Custom Links, we don't currently show an "Original" link. This ticket proposes that we add an Original link if the content is on-site, by adding a check to see where the content is, and if it's not add a "View" link that opens in a new tab/window.

Per the discussion today (see Slack link above), this is wontfix. If a user adds a Custom Link, there's no expectation of being able to click to see their content. They can always copy/paste the URL. While non-ideal, it works and it's functional. Additionally, there's nothing preventing them from clicking the link in the live preview of the menu and navigating to the on-site link.

#10 in reply to: ↑ 9 @westonruter
6 years ago

Replying to samuelsidler:

Replying to westonruter:

I just found a bug with the Original link: it is not populated for newly-added menu items.

Can you file a new ticket for that? (See also #32814.)

Done. See #32858.

Note: See TracTickets for help on using tickets.