Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Varia: Include Jetpack Content Options (#7518)" #7566

Closed
wants to merge 1 commit into from

Conversation

pbking
Copy link
Contributor

@pbking pbking commented Dec 20, 2023

This reverts commit 448a2ec.

This fixes the bug reported here: Automattic/wp-calypso#85369

Which was caused by using Jetpack to control varia (and children) Featured Image setting rather than Varia doing so. #7518

'archive' => true,
'archive-default' => true,
'post' => true,
'page' => true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is that I'm not sure whether get_theme_mod( 'show_featured_image_on_pages', false ) is saved, but if it is, would it not be possible to add page-default based on that setting here so that users are still able to access the benefits of Content Options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What else do the content options provide? I'm not familiar with what else this enables for the theme.

Copy link
Contributor

@Aurorum Aurorum Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, people were only able to hide Featured Images for pages. This led to requests of people asking to be able to hide Featured Images for posts and on their home page, as possible on other themes. Content Options gives users this ability (see the issues referenced in #7518!).

Content Options also allows control over how the blog is displayed, and I'm not sure if it'd be problematic for that to be changed for users, since that'd also be quite drastic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does default to "false" in the varia implementation.

So we would need to start there. And also the same for all of the other options; we can't turn on displaying the featured image without user intervention.

Also if the user had already set it to 'true' then we would need to send that through to the jetpack-content-options, so just defaulting to 'false' wouldn't be enough, that value would also have to include whatever value was stored in the customizer.

It seems overly complex to leverage Jetpack to manage the display of the featured image.

Copy link
Contributor

@Aurorum Aurorum Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we would need to start there. And also the same for all of the other options; we can't turn on displaying the featured image without user intervention.

This is what this PR would end up doing though. :) The users would have their Featured Images displayed without any control/consent over this.

Also if the user had already set it to 'true' then we would need to send that through to the jetpack-content-options, so just defaulting to 'false' wouldn't be enough, that value would also have to include whatever value was stored in the customizer.

Please forgive me if I'm misunderstanding here, but if that value is no longer stored, I don't see how this resolves Automattic/wp-calypso#85369? I think that the error which I made was not setting page-default to false.

It seems overly complex to leverage Jetpack to manage the display of the featured image.

That is fair enough, but I'd argue that this PR should implement all the options properly within Varia rather than just reverting the change (it still labels it as "Jetpack Content Options", after all!). It would probably be best if it addressed the issues that #7518 fixes.

@pbking
Copy link
Contributor Author

pbking commented Dec 20, 2023

A possible alternative: #7567

@pbking
Copy link
Contributor Author

pbking commented Dec 20, 2023

I tinkered with the alternative for a minute, but I'm still not keen on changing Varia at all to depend on Jetpack for this functionality.

Previously there was no dependency, now there is and any site that uses Varia MUST have Jetpack activated and the Content Options available for the feature previously available to work as expected.

Secondly there are a lot of child themes and for a change like this I would want an exhaustive review of what happens before and after this change for all of those child themes.

The original change has caused a lot of support issues and caused issues in quite a few live sites. I believe the time necessary to fully vet this change isn't appropriate to take.

I believe that we should merge this change, undoing the change that started using the Jetpack option. Doing so will return users to their previously configured state.

@Aurorum
Copy link
Contributor

Aurorum commented Dec 20, 2023

Previously there was no dependency, now there is and any site that uses Varia MUST have Jetpack activated and the Content Options available for the feature previously available to work as expected.

👍 My original reason for including Content Options was because it seemed like that what was intended to begin with, but it was just included incorrectly.

The original change has caused a lot of support issues and caused issues in quite a few live sites. I believe the time necessary to fully vet this change isn't appropriate to take.

I think this is because the original support issue conflated two issues - as mentioned in the issue, some aren't even related to Varia child themes. I still believe this change risks doing more harm than good, but just affecting a different subsection of users.

I believe that we should merge this change, undoing the change that started using the Jetpack option. Doing so will return users to their previously configured state.

Just to be clear, it won't, which is why I'm opposed to this change. It would result in Featured Images being enabled by default for posts and archives, and users having no control over this - this came up fairly regularly in support tickets, so there will definitely be frustrated users by this being reverted.

I think that removing Content Options here should include a proper fix for that issue, which offers a toggle for each content type.

@Aurorum
Copy link
Contributor

Aurorum commented Dec 20, 2023

@pbking - if you don't mind, would you mind letting me spin up a PR which I think would resolve both problems?

@pbking
Copy link
Contributor Author

pbking commented Dec 20, 2023

Not at all! Please do and I'll take it for a spin.

@pbking
Copy link
Contributor Author

pbking commented Dec 20, 2023

I've actually re-opened #7567 after making a small change.

I went back and forth (pre change, current change, this change) with various scenarios of changing the customizer setting(s).

I think you have convinced me. I'm keen to see what your PR contains, though if you're keen to ship #7567 I am too.

@pbking
Copy link
Contributor Author

pbking commented Dec 20, 2023

Closing this, #7567 is closer to desired behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants