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

Jetpack Testimonials: Missing Priority for Theme Customizer Integration #538

Closed
philiparthurmoore opened this Issue May 14, 2014 · 7 comments

Comments

Projects
None yet
6 participants
@philiparthurmoore
Copy link

philiparthurmoore commented May 14, 2014

There's no way to set priority on the jetpack_testimonials section when theme support for Jetpack Testimonials is added into a theme, which means that the Customizer section for Jetpack Testimonials is fully controlled by Jetpack, and at present always displayed at the very top of the Customizer, even above Site Title & Description:

This is problematic for a number of reasons:

  1. When a theme is activated or previewed there's no context or instructions for why that section is in the Customizer. It's confusing, and the options set for it is non-consequential for the overall usability and functionality of the theme. None of the options are very important there, but its position in the Customizer makes it seem like it's important.
  2. There's no way for its position within the Customizer to be adjusted by the theme or for additional theme options within the Customizer to be placed in relative positions to Jetpack Testimonials to make better usability sense to the user.

I can think of two possible solutions:

  1. Allow theme authors to declare priority when theme support is added.
  2. Add a sensible priority directly into Jetpack that puts Jetpack Testimonials into a more logical position.

Happy to submit a patch for either approach pending feedback from the Theme Team.

@jeherve jeherve added Theme Tools and removed bug labels May 14, 2014

@BinaryMoon

This comment has been minimized.

Copy link
Contributor

BinaryMoon commented May 14, 2014

Plus one on this. I think Jetpack should just pick a more sensible priority.

@philiparthurmoore

This comment has been minimized.

Copy link

philiparthurmoore commented May 18, 2014

I was wrong about this, and was not fully thinking about the possibility of theme authors being able to manipulate this section with $wp_customize->get_section. The way that I've gone about manipulating customizer integration so that theme authors are able to take advantage of CPTs is as follows. I do not know if this is the best approach, and would appreciate feedback. I still think that Jetpack needs a more sensible approach to integrating Testimonials into the Customizer, so I'd like this Issue to be left open until the Theme Team addresses the issue of the section's default position.

Here's an example of how I've gone about adjusting Testimonials:

    /**
     * Testimonials tweaks, but only if Jetpack is active and the Testimonials class exists
     *
     * @uses function get_section
     *
     * @since Theme Name 1.0
     */
    if ( class_exists( 'Jetpack' ) && class_exists( 'Jetpack_Testimonial' ) ) {
        // Better title for testimonials, for more clarity
         $wp_customize->get_section( 'jetpack_testimonials' )->title = __( 'Customer Testimonials', 'theme-name' );
        // Give this seciton a more sensible priority
         $wp_customize->get_section( 'jetpack_testimonials' )->priority = 100;
    }

I've put this inside a function hooked to customize_register, with a priority set to 11 so that we're able to tweak testimonials, i.e.

add_action( 'customize_register', 'theme_name_customize_register', 11 );

Are there any potential issues with this approach? Seems fine to me; no errors or issues as I've debugged along the way.

/cc @michaeldcain

@philiparthurmoore

This comment has been minimized.

Copy link

philiparthurmoore commented Jun 27, 2014

There's been no response on this for quite some time. Should I just submit a patch?

@kraftbj

This comment has been minimized.

Copy link
Contributor

kraftbj commented Jun 27, 2014

@philiparthurmoore If you have code ready, a PR would likely be a faster way. @obenland, thoughts?

@obenland

This comment has been minimized.

Copy link
Member

obenland commented Jun 27, 2014

I agree with @philiparthurmoore, we should add a lower priority (higher number) to bump it down after the core-registered sections. This shouldn't be something themes have to worry about.

Unrelated: The theme support check here should be handled through the theme_supports argument to the add_setting() method.

@michaeldcain

This comment has been minimized.

Copy link
Member

michaeldcain commented Aug 7, 2014

@philiparthurmoore, I agree that a better priority should be set by the CPT (I've switched it to 130 in the PR above), and I don't see an issue with your code above in situations where a theme would want to edit the CPT labels or priority on top of that.

@philiparthurmoore

This comment has been minimized.

Copy link

philiparthurmoore commented Aug 8, 2014

@michaeldcain: Patch looks great. Thanks for adding this in!

@jeherve jeherve added this to the 3.2 milestone Aug 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment