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

Deprecate the constant SHOW_STICKY_NAV #1136

Merged
merged 9 commits into from
Apr 6, 2016

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Feb 16, 2016

Changes

  • Notes that SHOW_STICKY_NAV is deprecated in the constants list in the dev docs
  • Since the theme option show_sticky_nav was removed in Rework navigation elements #1024, uses sticky_nav_display_article as an approximation of whether or not the sticky nav should be displayed. This is a temporary fix, and is not meant to accurately reflect whether or not the sticky nav should be displayed. This is only to avoid the sticky nav never displaying. this PR defines SHOW_STICKY_NAV as false.
  • Removes the update function for show_sticky_nav in inc/updates.php
  • Removes the test for the update function's show_sticky_nav updater
  • Marks largo_sticky_nav_active as deprecated as well, because that function existed to return SHOW_STICKY_NAV. largo_sticky_nav_active has no tests.

Why

This is a compatibility fix, to ensure that sites still using SHOW_STICKY_NAV will continue to work.

Because SHOW_STICKY_NAV no longer is an accurate descriptor of whether or not the sticky nav should be displayed, and because the decision on whether or not to show the sticky nav is made by navigation.js since #1024.

We're keeping SHOW_STICKY_NAV to avoid undefined constant errors in child themes and giving it a value that isn't false so that the navigation will continue to display, to avoid problems on sites using SHOW_STICKY_NAV. HELPDESK-568 is such a site.

See #1135 for the non-hotfix solution for this issue:

  • check all child themes for SHOW_STICKY_NAV and remove those conditionals if and only if the child theme does not define SHOW_STICKY_NAV to true.
  • mention that this is deprecated in the 0.5.5 release notes.

For HELPDESK-568 and #1135

@benlk benlk added priority: high Either blocks work on a priority-normal task or a solution here informs other work. status: needs review labels Feb 16, 2016
@benlk benlk added this to the hotfix milestone Feb 16, 2016
… Largo, and now only returns the value of SHOW_STICKY_NAV, which is itself deprecated.
@aschweigert
Copy link

This should probably not be a hotfix because it would be a breaking change for sites that are currently using this constant in a child theme (right?)

@benlk
Copy link
Collaborator Author

benlk commented Feb 16, 2016

It's a breaking change in that it's a change from the previous functionality, because the conditions regarding SHOW_STICKY_NAV change, but if their theme is currently working with SHOW_STICKY_NAV it's either because their child theme sets SHOW_STICKY_NAV or because they haven't saved their theme options after running the update function when they updated from 0.5.4.

The current functionality is broken if the child theme doesn't define SHOW_STICKY_NAV:

  • There is no theme option controlling the value of the theme option show_sticky_nav.
  • If the last time theme options were saved was when update functions were run on 0.5.4, then SHOW_STICKY_NAV is true because the update function set show_sticky_nav to true. The net time they save these options, it will become false.
  • If they saved theme options since then, SHOW_STICKY_NAV is false because show_sticky_nav is not set.

This PR makes SHOW_STICKY_NAV at least configurable.

@benlk benlk modified the milestones: 0.5.5 - Story Elements, hotfix Feb 16, 2016
@benlk benlk added priority: normal Must be completed before release of this version of plugin. and removed priority: high Either blocks work on a priority-normal task or a solution here informs other work. labels Feb 16, 2016
@benlk
Copy link
Collaborator Author

benlk commented Feb 16, 2016

Based on discussion, we're leaving SHOW_STICKY_NAV as false, period.

This will be deprecated in 0.5.5, and at that time we should go through child themes and remove their dependence upon SHOW_STICKY_NAV if the child theme does not set it to true.

@aschweigert
Copy link

it will be deprecated in 0.5.6. 0.5.5 will restore the expected behavior from pre 0.5.4 until we communicate with sites using these constants re: how the change will affect them

@benlk
Copy link
Collaborator Author

benlk commented Feb 16, 2016

A misphrasing on my part, I guess. This PR:

  • maintains pre-0.5.4 behavior in sites that define SHOW_STICKY_NAV in their child theme
  • provides a predictable fallback for sites using SHOW_STICKY_NAV but not defining it: SHOW_STICKY_NAV is false
  • does not re-create the show sticky nav option in the theme options that is required for that to be configured, since Largo doesn't use that "globally show"/"globally don't-show" paradigm anymore, but replaces that paradigm with "globally don't-show"/"conditionally-show with Javascript"
  • does not affect the behavior of sites not using SHOW_STICKY_NAV

We should've marked SHOW_STICKY_NAV as deprecated in 0.5.4 when we removed the theme option controlling it, but it's too late to do that.

Should this specific PR be bumped to 0.5.6, and in 0.5.5 we try to approximate the show/don't-show logic from js/navigation.js' in PHP for the constant?

@benlk
Copy link
Collaborator Author

benlk commented Apr 1, 2016

None of our Largo child themes use SHOW_STICKY_NAV or largo_sticky_nav_active, though:

  • City Limits does define SHOW_STICKY_NAV to be false. It has no effect.
  • Current has a comment about it, but doesn't use it. I'm going to remove the comment from their theme, because the comment is false and misleading.

@benlk benlk removed their assignment Apr 1, 2016
@aschweigert aschweigert merged commit ecb501f into master Apr 6, 2016
@aschweigert aschweigert deleted the 1135-SHOW_STICKY_NAV-deprecation branch April 6, 2016 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: normal Must be completed before release of this version of plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants