Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.42k stars 1.99k forks source link

Site Settings: Make infinite scroll "Click-to-scroll" when site has footer widgets #13637

Open tyxla opened 7 years ago

tyxla commented 7 years ago

In Site Settings we should take into consideration the case when a site has footer widgets, and display a message accordingly, instead of allowing the user to change the settings. Further details can be read here: https://github.com/Automattic/wp-calypso/issues/13569#issuecomment-298735689

Here is how it looks now:

cc @rickybanister @MichaelArestad can you folks draft a quick mockup of how the Infinite Scroll settings should look in that case?

rickybanister commented 7 years ago

It occurs to me that the wp-admin way of doing this does not give the user much choice. If they have footer widgets we can still give them two radio options.

I noticed that the infinite scroll card is missing the site-settings__module-settings class, so when I added 'is-disabled' to the last option it didn't properly change the opacity. If you could add that or whatever necessary classes to that card, that'd be awesome.

Here's a drawing for you:

image

tyxla commented 7 years ago

Thanks for that, @rickybanister!

Okay, now we have a technical problem to solve. Basically, each theme defines sidebars for itself, and not necessarily in a consistent way. For example, the Argent theme has 3 footer sidebars with IDs sidebar-1, sidebar-2, and sidebar-3. Other themes use the same sidebar IDs, but for main sidebars, and not for the footer one. So, how can we know if the user has widgets in the footer sidebar, if we don't know which sidebar is a footer sidebar and which one isn't?

CC @aduth for a more experienced opinion and @sirbrillig as he worked on the /sites/$site/widgets endpoint.

sirbrillig commented 7 years ago

So, how can we know if the user has widgets in the footer sidebar, if we don't know which sidebar is a footer sidebar and which one isn't?

This isn't a very helpful answer, but I'm pretty sure you can't. Themes can do whatever they want and there's no reliable way to mark a sidebar as being a "footer" sidebar. The sidebar's name is the only clue we have, and that can be inaccurate.

To solve this sort of impossible problem in other places, we've used the idea of theme annotations: specific configuration files for each theme which contain meta-data used to declaratively describe certain parts of that theme. In this case we could create an annotation file for each theme that listed each of that theme's sidebars and labeled the ones which were footers. Not a short project, but IMO it's the only reasonable way (until themes are built of content blocks).

(It's possible there could be another way, which I'd love to hear!)

MichaelArestad commented 7 years ago

I would posit that there is no good way in the near future to tell if a theme has a footer that won't be seen with infinite scroll. Also, what may have been a sidebar on desktop often ends up as a footer on mobile. I would put a short message explaining the possibility in all cases.

laurelfulford commented 7 years ago

So, how can we know if the user has widgets in the footer sidebar, if we don't know which sidebar is a footer sidebar and which one isn't?

Is it possible to check the current theme’s infinite scroll settings, and if it’s filtering infinite_scroll_has_footer_widgets?

Each theme should have the footer widget IDs listed in the infinite scroll settings, like:

add_theme_support( 'infinite-scroll', array(
    'container'      => 'main',
    'footer'         => 'page',
    'footer_widgets' => array(
        'sidebar-2',
        'sidebar-3',
    ),
) );

Some themes also check whether there’s a footer menu, and include that in the IS settings to switch to click-to-scroll. This is done either by creating a custom function that returns true or false and assigning it to footer_widgets, or by filtering infinite_scroll_has_footer_widgets.

Also, what may have been a sidebar on desktop often ends up as a footer on mobile.

This is addressed in some WP.com themes, again by filtering infinite_scroll_has_footer_widgets and checking the device with jetpack_is_mobile():

if ( function_exists( 'jetpack_is_mobile' ) && ! function_exists( 'penscratch_2_has_footer_widgets' ) ) {

    function penscratch_2_has_footer_widgets() {
        if ( is_active_sidebar( 'sidebar-2' ) || is_active_sidebar( 'sidebar-3' ) || is_active_sidebar( 'sidebar-4' ) || has_nav_menu( 'jetpack-social-navigation' ) ||  ( ( jetpack_is_mobile( '', true ) && is_active_sidebar( 'sidebar-1' ) ) ) )
            return true;

        return false;
    }
} //endif
add_filter( 'infinite_scroll_has_footer_widgets', 'penscratch_2_has_footer_widgets' );

Not all themes include a check for this, though; I'm going to dig into that a bit more.

tyxla commented 7 years ago

@laurelfulford oh, that is nice!

If the infinite_scroll_has_footer_widgets is widely used within themes, it would make sense to take advantage from it. We will probably have to build a new endpoint for that though - I don't think that there is an endpoint that allows to retrieve the supported features by the current theme - cc @ockham for confirmation.

Still, can we get some stats about the themes that define the footer widgets usage this way? Who can we ask about that? If there aren't much themes that use it, it will probably not make sense to bother with that at all.

laurelfulford commented 7 years ago

@tyxla I did a quick search of the themes on WP.com -- there are forty live themes that use infinite_scroll_has_footer_widgets, plus four retired themes that use it.

Just let me know if there's any more information I can provide that would be useful (like whether these themes use infinite_scroll_has_footer_widgets for the sidebar widgets on small screens, menus in the footer area, or actual footer widgets).

tyxla commented 7 years ago

Wow, thanks a ton @laurelfulford!

tyxla commented 7 years ago

cc folks from @Automattic/jetpack as this will have to be implemented in Jetpack, too.

zinigor commented 7 years ago

Thanks for working on that! So currently Jetpack automatically enables click mode when a theme has footer widgets. So the only thing we need to improve is the setting here.

ockham commented 7 years ago

Sry @tyxla, missed this convo so far.

If the infinite_scroll_has_footer_widgets is widely used within themes, it would make sense to take advantage from it. We will probably have to build a new endpoint for that though - I don't think that there is an endpoint that allows to retrieve the supported features by the current theme - cc @ockham for confirmation.

Originally answered at p1HpG7-427-p2#comment-21865:

just look up the theme from the sites/<myjetpacksite>/themes endpoint and check the result's taxonomies.theme_feature array? If this is about Calypso, that's as easy as using a <QueryTheme /> component and a getTheme() selector.

ockham commented 7 years ago

If you need to determine the current theme first: <QueryActiveTheme /> and getActiveTheme().

zinigor commented 7 years ago

I just had an idea. In Jetpack we default to the "Click" mode only when the footer widgets are enabled, not only supported. So to be sure that we need to grey out that settings we'll need the additional data about sidebars, not only theme features.

So, why don't we display a conditional warning here in case a theme supports footer widgets, but allow the user to set it to "infinite scroll" mode anyway. The warning will read something like this:

"You currently have an active theme that supports footer widgets. Keep in mind that if you enable them, Infinite Scroll will only work in 'button' mode."

tyxla commented 7 years ago

@ockham as discussed in the p2 post, I don't think this approach will work for custom themes. But perhaps I'm missing something?

tyxla commented 7 years ago

A precise implementation requires us to build a new endpoint to retrieve the current theme's supported features. Slating this to v2 for the same reason as #14912.

davidakennedy commented 5 years ago

This came up in 2132926-zen. Related theme issue was filed here and closed because this is more of a settings issue. See: https://github.com/Automattic/themes/issues/1021

github-actions[bot] commented 3 years ago

This issue is stale because it has been 180 days with no activity. You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation and apply one of relevant issue close labels.

github-actions[bot] commented 2 years ago

This issue is stale because it has been 180 days with no activity. You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation and apply one of relevant issue close labels.

tyxla commented 2 years ago

I'm leaving it open as I'm not convinced it's no longer useful.

github-actions[bot] commented 2 years ago

This issue is stale because it has been 180 days with no activity. You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation and apply one of relevant issue close labels.