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

Refactor config files #264

Merged
merged 7 commits into from
Jun 14, 2019
Merged

Refactor config files #264

merged 7 commits into from
Jun 14, 2019

Conversation

nickcernis
Copy link
Collaborator

Fixes #260.

We can retrieve the default color from the appearance config instead.

We don't have getter functions to retrieve other preferences, so it
seems unnecessary to have one for the colors alone.
@nickcernis nickcernis requested a review from dreamwhisper June 13, 2019 16:18
@nickcernis
Copy link
Collaborator Author

Noting two potential improvements for the future (that each require changes in Genesis):

  • Move responsive-menus.php into theme-supports.php (would require Genesis to allow genesis_register_responsive_menus support to be declared via a theme support instead of the current function call, which would make more sense to me anyway).
  • Remove genesis_sample_theme_support from functions.php and have Genesis autoload the theme-supports.php file

This allows them to be referenced inside the returned array, preventing
a PHP warning.
'color' => get_theme_mod( 'genesis_sample_accent_color', genesis_sample_get_default_color( 'accent' ) ),
'color' => get_theme_mod(
'genesis_sample_accent_color',
$genesis_sample_default_colors['accent']
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be defined earlier like the link color for consistency?

$genesis_sample_accent_color = get_theme_mod(
	'genesis_sample_accent_color',
	$genesis_sample_default_colors['accent']
);

@@ -18,6 +27,7 @@
'default-button-bg' => $genesis_sample_link_color,
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't have to be for this PR, but for each default- here, they aren't always "default" once the colors are changed. Perhaps we should look at renaming these?

@nickcernis
Copy link
Collaborator Author

@dreamwhisper Thanks for your review. I like both changes you suggested and have updated this PR.

This ensures widget areas get added.
@dreamwhisper dreamwhisper merged commit c9b4c44 into develop Jun 14, 2019
@dreamwhisper dreamwhisper deleted the refactor/config branch June 14, 2019 13:10
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.

Refactor config folder
2 participants