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

Remove ConditonalTagCheck class #1494

Merged
merged 1 commit into from
Sep 23, 2015
Merged

Remove ConditonalTagCheck class #1494

merged 1 commit into from
Sep 23, 2015

Conversation

QWp6t
Copy link
Member

@QWp6t QWp6t commented Jun 12, 2015

Update

This PR began as a rewrite of ConditionalTagCheck as a function instead of class, but we then decided to get rid of callback support, which alleviated the need for a separate function (or class). It now simply uses PHP's native in_array() function.

If you're viewing this PR because you're looking for guidance on how to control when the sidebar is displayed, you can view the updated docs on roots.io.

Original Post

Note that config.php now encourages users to use the following syntax:

      is_404(),
      is_front_page(),
      is_page_template('template-custom.php'),

The old method of using an array of callbacks is still supported if for whatever reason someone still wants to do that, but I don't see the point in using that method.

Honestly I think we could just remove the callback support all together and replace display_sidebar() with:

   static $display;

   if (!isset($display)) {
-    $display = !Utils\conditional_reduction([
+    $display = !in_array(true, [
       /**
        * Any of these conditional tags that return true won't show the sidebar.
        * You can also specify your own callbacks as long as they each return a boolean.
        *
        * Examples:
        *
        * is_single()
        * is_archive()
        * is_page('about-me')
        * is_tax(['flavor', 'mild'])
        * is_page_template('about.php')
        * is_post_type_archive(['foo', 'bar', 'baz'])
        * \MyNamespace\MyClass::MyMethod(['my', 'args'])
        *
        */
       is_404(),
       is_front_page(),
       is_page_template('template-custom.php'),
     ]);
   }

   return apply_filters('sage/display_sidebar', $display);
 }

@swalkinshaw
Copy link
Member

I'm fine with removing callback support. Anyone can just do something like:

$display = $display || my_custom_check();

@QWp6t
Copy link
Member Author

QWp6t commented Jun 17, 2015

So what's the verdict? Are we dropping it entirely?

It was kinda fun refactoring the function, but I won't miss it if it's gone. I don't think anyone else will either. I can't imagine any users being terribly attached to the awkward syntax with the arrays.

@swalkinshaw
Copy link
Member

@QWp6t drop it

@QWp6t QWp6t force-pushed the QWp6t-conditionaltagcheck branch 2 times, most recently from d5b14ac to c2707bc Compare July 9, 2015 18:44
@QWp6t
Copy link
Member Author

QWp6t commented Jul 9, 2015

👌

@QWp6t QWp6t force-pushed the QWp6t-conditionaltagcheck branch from c2707bc to 1aabb05 Compare July 13, 2015 01:54
@QWp6t
Copy link
Member Author

QWp6t commented Jul 13, 2015

I added a couple other changes.

  • $display is no longer a static variable. Developers can modify the function as needed if they need to cache results.
  • Examples have been removed. Since the function itself has been simplified, I think the large block of docs would overwhelm new developers, i.e., make them think this is more complicated than it actually is.

I believe these changes are more consistent with our efforts to declutter and minimize Sage.

@JulienMelissas
Copy link
Contributor

So much simpler and easy to use. I'm all for this one. Thanks @QWp6t 💟

@Foxaii
Copy link
Contributor

Foxaii commented Jul 13, 2015

I don't think we should drop the static nature of the variable. Some conditionals can vary their return depending on where in the page load process (pre loop, in the loop, post loop) they are, so this could lead to some inconsistencies.

@QWp6t
Copy link
Member Author

QWp6t commented Jul 13, 2015

That's initially why I added the static variable back in #1196, but you could just as easily hook into sage/display_sidebar filter to accomplish the same thing or modify the function to set your own static variable.

As I mentioned above, I think the change is consistent with our recent moves toward decluttering and minimizing Sage. By default we don't need it to be static, and I think most users don't need that either.

But obviously I'll add the static variable back if everyone agrees it should be there. lol

@swalkinshaw
Copy link
Member

Just a note that we're going to require some docs updates after this: https://roots.io/sage/docs/theme-sidebar/

@Foxaii
Copy link
Contributor

Foxaii commented Jul 19, 2015

I'm still very much against dropping the static variable.

The only benefit of removing it is it won't be there. Keeping it improves performance by not unnecessarily repeating a bunch of conditional calls and guarantees the function returns consistently, which is a real issue considering some conditionals vary throughout the load process.

Dropping one keyword makes it less performant and less reliable. That's not the kind of decluttering we should be doing.

@QWp6t QWp6t force-pushed the QWp6t-conditionaltagcheck branch from 1aabb05 to 86f72f7 Compare August 11, 2015 19:37
@QWp6t
Copy link
Member Author

QWp6t commented Aug 11, 2015

@Foxaii 🍉

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@QWp6t QWp6t force-pushed the QWp6t-conditionaltagcheck branch from 86f72f7 to 6d05c67 Compare August 12, 2015 01:47
@QWp6t QWp6t changed the title Replace ConditonalTagCheck class with function Remove ConditonalTagCheck class Sep 2, 2015
retlehs added a commit that referenced this pull request Sep 23, 2015
@retlehs retlehs merged commit a6c9d4d into master Sep 23, 2015
@retlehs retlehs deleted the QWp6t-conditionaltagcheck branch September 23, 2015 01:21
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.

None yet

5 participants