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

Enable status button if current theme is not active. #132

Merged
merged 15 commits into from Mar 21, 2017

Conversation

Projects
None yet
3 participants
@sayedtaqui
Collaborator

sayedtaqui commented Mar 11, 2017

Fixes #131

@sayedtaqui

This comment has been minimized.

Collaborator

sayedtaqui commented Mar 11, 2017

@westonruter We have a param theme=twentyfifteen in url when we switch theme, I think that should also be saved in changeset like we saved the history query params in the other PR?

@westonruter

This comment has been minimized.

Contributor

westonruter commented Mar 11, 2017

@sayedtaqui oh yes, absolutely! That param can be included in the customizer state query params only when saving a changeset for a theme that is not active. If the theme is active, then the param should be omitted from being saved.

@westonruter westonruter added this to the 0.6.0 milestone Mar 11, 2017

@coveralls

This comment has been minimized.

coveralls commented Mar 13, 2017

Coverage Status

Coverage decreased (-0.03%) to 76.537% when pulling 023a0a6 on bugfix/issue-131 into c7b9124 on develop.

@coveralls

This comment has been minimized.

coveralls commented Mar 13, 2017

Coverage Status

Coverage decreased (-0.03%) to 76.537% when pulling a372282 on bugfix/issue-131 into c7b9124 on develop.

@coveralls

This comment has been minimized.

coveralls commented Mar 13, 2017

Coverage Status

Coverage decreased (-0.1%) to 76.431% when pulling c71add1 on bugfix/issue-131 into c7b9124 on develop.

@coveralls

This comment has been minimized.

coveralls commented Mar 14, 2017

Coverage Status

Coverage decreased (-0.1%) to 76.431% when pulling 58f0ce9 on bugfix/issue-131 into c7b9124 on develop.

@westonruter

This comment has been minimized.

Contributor

westonruter commented Mar 14, 2017

Is this out of WIP and ready for review?

queryVars.scroll = parseInt( api.previewer.scroll, 10 ) || 0;
queryVars.device = api.previewedDevice.get();
queryVars.url = api.previewer.previewUrl.get();
if ( ! api.state( 'activated' ).get() || snapshot.data.theme !== currentTheme ) {
queryVars.previewingTheme = true;

This comment has been minimized.

@westonruter

westonruter Mar 14, 2017

Contributor

Since this is going to be sent with the POST request, I think it should be previewing_theme instead. Then below in PHP, you'd read $query_vars['previewing_theme'].

if ( isset( $preview_url_query_vars['theme'] ) && $current_theme !== $preview_url_query_vars['theme'] ) {
$args = array_merge( $args, array(
'theme' => $preview_url_query_vars['theme'],

This comment has been minimized.

@westonruter

westonruter Mar 14, 2017

Contributor

Extra tab here?

@sayedtaqui

This comment has been minimized.

Collaborator

sayedtaqui commented Mar 15, 2017

Not ready yet, need to test and may be few changes. Trying to finish today.

@coveralls

This comment has been minimized.

coveralls commented Mar 15, 2017

Coverage Status

Coverage decreased (-0.2%) to 76.388% when pulling cc2f54d on bugfix/issue-131 into c7b9124 on develop.

@coveralls

This comment has been minimized.

coveralls commented Mar 15, 2017

Coverage Status

Coverage decreased (-0.2%) to 76.388% when pulling 26f7316 on bugfix/issue-131 into c7b9124 on develop.

@sayedtaqui sayedtaqui changed the title from [WIP]Enable status button if current theme is not active. to Enable status button if current theme is not active. Mar 15, 2017

@sayedtaqui

This comment has been minimized.

Collaborator

sayedtaqui commented Mar 15, 2017

Ready for review.

@@ -255,6 +266,7 @@
dialogElement;
snapshot.statusButton.disableSelect.set( false );
snapshot.statusButton.disbleButton.set( false );

This comment has been minimized.

@westonruter

westonruter Mar 16, 2017

Contributor

I'm glad you caught this. It's something I was noticing in the case of setting validation errors, where I'd get a validation error message with a control but the Publish button would remain disabled.

@coveralls

This comment has been minimized.

coveralls commented Mar 16, 2017

Coverage Status

Coverage decreased (-26.3%) to 50.233% when pulling 2559457 on bugfix/issue-131 into c7b9124 on develop.

Ensure frontend preview link gets customize_theme param in theme swit…
…ches

The 'customize_theme' param is used on the frontend, whereas just 'theme' is used in the admin.
@westonruter

This comment has been minimized.

Contributor

westonruter commented Mar 16, 2017

@sayedtaqui the one remaining issue I see, and this isn't strictly related to the changes here, but when I publish the changeset the frontend preview link continues to have the customize_changeset_uuid even though it is published:

image

@coveralls

This comment has been minimized.

coveralls commented Mar 16, 2017

Coverage Status

Coverage decreased (-26.3%) to 50.233% when pulling 9ca3ff2 on bugfix/issue-131 into c7b9124 on develop.

@@ -106,7 +101,7 @@ public function enqueue_controls_scripts() {
'initialServerDate' => current_time( 'mysql', false ),
'initialServerTimestamp' => floor( microtime( true ) * 1000 ),
'theme' => $this->original_stylesheet,
'previewingTheme' => isset( $preview_url_query_vars['theme'] ) ? $preview_url_query_vars['theme']: '',
'previewingTheme' => ! $this->customize_manager->is_theme_active(),

This comment has been minimized.

@sayedtaqui

sayedtaqui Mar 21, 2017

Collaborator

@westonruter This is breaking my condition here

snapshot.isNotSavedPreviewingTheme = savedPreviewingTheme && savedPreviewingTheme !== currentTheme;

There are several use cases where the button has to be enabled or disabled.

  1. When they switch theme ( Has to enable ) ( Working )
  2. After saving the previewing theme, reload the page, ( Has to disable ) ( Not working now )
  3. After saving the previewing theme they switch to a third theme ( Has to enable ) ( Working )
  4. After travelling through the themes and saving the last one, they come back to the original theme ( Should be enable ) ( Not working now ).

I was not able to satisfy these conditions with only boolean value of previewingTheme . This is as good as api.state( 'activated' ).get()

This comment has been minimized.

@westonruter

westonruter Mar 21, 2017

Contributor

Thanks for pointing this out. So this can be fixed by reverting back to:

'previewingTheme' => isset( $preview_url_query_vars['theme'] ) ? $preview_url_query_vars['theme'] : '',

?

This comment has been minimized.

@sayedtaqui

sayedtaqui Mar 21, 2017

Collaborator

Yes.

@coveralls

This comment has been minimized.

coveralls commented Mar 21, 2017

Coverage Status

Coverage decreased (-26.3%) to 50.233% when pulling a71adbb on bugfix/issue-131 into c7b9124 on develop.

@@ -225,7 +226,7 @@ public function add_snapshot_uuid_to_return_url() {
&&
$this->current_snapshot_uuid
&&
$this->is_theme_active()
$this->customize_manager->is_theme_active()
&&

This comment has been minimized.

@sayedtaqui

sayedtaqui Mar 21, 2017

Collaborator

@westonruter Not sure why $this->customize_manager->is_theme_active() is returning false in 4.6.1 which is failing phpunit test. I mean inside phpunit test.

This comment has been minimized.

@westonruter

westonruter Mar 21, 2017

Contributor

It's because the setup_theme action wasn't triggered in the test case, and so \WP_Customize_Manager::$original_stylesheet didn't get set. In 4.7 this variable gets set in \WP_Customize_Manager::__construct() whereas in 4.6 it got set in \WP_Customize_Manager::setup_theme().

This comment has been minimized.

@westonruter

westonruter Mar 21, 2017

Contributor

Should be fixed in 52694b6.

@coveralls

This comment has been minimized.

coveralls commented Mar 21, 2017

Coverage Status

Coverage decreased (-26.4%) to 50.175% when pulling eb003e3 on bugfix/issue-131 into c7b9124 on develop.

@coveralls

This comment has been minimized.

coveralls commented Mar 21, 2017

Coverage Status

Coverage decreased (-26.4%) to 50.175% when pulling 52694b6 on bugfix/issue-131 into c7b9124 on develop.

@westonruter westonruter merged commit a255424 into develop Mar 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the bugfix/issue-131 branch Mar 21, 2017

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