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

Theme settings for all the default WordPress themes #831

Merged
merged 7 commits into from Mar 16, 2016

Conversation

Projects
None yet
3 participants
@marcin-lawrowski
Copy link
Contributor

commented Mar 14, 2016

Fixes #828

),
'twentyeleven_theme_options' => array(

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 14, 2016

Contributor

Why do we include a second array of labels here? How are twentyeleven options different?

This comment has been minimized.

Copy link
@marcin-lawrowski

marcin-lawrowski Mar 14, 2016

Author Contributor

Twentyeleven theme has "Twenty Eleven Theme Options" page in Appearance menu and settings listed there are saved in "twentyeleven_theme_options" option

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 14, 2016

Contributor

What do you think of this - just saving a generic "Twenty Eleven Theme Options" setting - instead of going into detail?

I'm not sure it's worth it for the added code complication.

What do you think?

@@ -550,7 +577,8 @@ function( $rule ) use ( $submenu, $record ) {
* @param mixed $value
*/
public function callback_update_option( $option, $value, $old_value ) {
if ( ( defined( '\WP_CLI' ) && \WP_CLI || did_action( 'customize_save' ) ) && array_key_exists( $option, $this->labels ) ) {
$is_theme_option = sprintf( '%s_theme_options', get_option( 'stylesheet' ) ) == $option;
if ( ( defined( '\WP_CLI' ) && \WP_CLI || did_action( 'customize_save' ) ) && ( $is_theme_option || array_key_exists( $option, $this->labels ) ) ) {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 14, 2016

Contributor

This statement is looking pretty messy. Can we clean it up?

@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2016

Got these two when updating Media settings (for the first time):
medium_large_size_w
image_default_link_type

What would they be referring to?

@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2016

I also got heaps of these ones, when updating the "Front Page displays" setting for the first time:

"([0-9]{4})/([0-9]{1,2})/([0-9]{1,2})/page/?([0-9]{1,})/?$" setting was updated

Can we exclude all settings that end with a $?

@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

In the Stream settings (also part of the Settings adaptor), I'm getting duplicate entries for the Keep Records For Setting. It appears like this:

screen shot 2016-03-15 at 09 59 52

marcin-lawrowski added some commits Mar 15, 2016

@marcin-lawrowski

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

Options "medium_large_size_w" and "image_default_link_type" are reset to empty string every time Media settings are saved because there is no input fields to edit them and they are present in $whitelist_options. I assume they are either internal or legacy. I wrote a code that ignores these options.

@marcin-lawrowski

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

As for the duplicated entries - when you select "Keep Records Indefinitely" then "Keep Records for" setting is cleared. When you clear "Keep Records Indefinitely" checkbox then "Keep Records for" setting is set to 30 days (default). So, when you update "Keep Records Indefinitely" setting then "Keep Records for" is updated as well.

$ignored = array(
'image_default_link_type',
'medium_large_size_w',
'medium_large_size_h',

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 15, 2016

Contributor

Is there such a thing as small_medium_size_w or other size variations?

This comment has been minimized.

Copy link
@marcin-lawrowski

marcin-lawrowski Mar 16, 2016

Author Contributor

No, there is not. "medium_large_size_w" and "medium_large_size_w" are the only options in this case.

@@ -333,6 +333,10 @@ public function is_option_ignored( $option_name ) {
return true;
}
if ( substr( $option_name, -1 ) === '$' ) {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Mar 15, 2016

Contributor

Yoda conditional, this should be.

This comment has been minimized.

Copy link
@marcin-lawrowski

marcin-lawrowski Mar 16, 2016

Author Contributor

Done

@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

@marcin-lawrowski Re: Duplicated entries:

Perhaps an easy and less confusing solution would be to simply change the label for "Keep Records for" to "Record TTL". The Stream Settings screen would remain the same. Thoughts?

@chacha

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

Just to pipe in...

Perhaps an easy and less confusing solution would be to simply change the label for "Keep Records for" to "Record TTL".

@lukecarbis I think that TTL is a technical phrase that I wouldn't expect many people to understand.

When you clear "Keep Records Indefinitely" checkbox then "Keep Records for" setting is set to 30 days (default). So, when you update "Keep Records Indefinitely" setting then "Keep Records for" is updated as well.

@marcin-lawrowski Two alternatives that pop up in my head:

  • Keep the value of "Keep Records for" and simply use JS to hide the option when indefinitely is selected. Internally just have the indefinite checkbox override.
  • Don't store the indefinite value and instead, replace "Keep Records for" with a 0 to indicate an indefinite time period.
@marcin-lawrowski

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2016

@chacha I also think that TTL phrase could be too difficult, so I decided to hide "Keep Records for" option if the checkbox is selected. Now there is only one Stream log when an user selects / deselects the checkbox.

lukecarbis pushed a commit that referenced this pull request Mar 16, 2016

Luke Carbis
Merge pull request #831 from xwp/bugfix/issue-828
Theme settings for all the default WordPress themes

@lukecarbis lukecarbis merged commit 7c9e8a7 into develop Mar 16, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@lukecarbis lukecarbis deleted the bugfix/issue-828 branch Mar 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.