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

TwigExtension - detect (in)human agent #1209

Closed
wants to merge 44 commits into from
Closed

Conversation

gigago
Copy link
Contributor

@gigago gigago commented Dec 13, 2016

Added function ishuman() to TwigExtension.

Incorrect comment updated.
Add function ishuman() to detect agent disposition.
@flaviocopes
Copy link
Contributor

The change in system/src/Grav/Common/Taxonomy.php is not related I think?

Also, please update your branch as it seems there are some conflicts with the current develop.

# Conflicts:
#	system/src/Grav/Common/Taxonomy.php
#	system/src/Grav/Common/Twig/TwigExtension.php
@gigago gigago closed this Dec 16, 2016
@gigago gigago reopened this Dec 16, 2016
@gigago
Copy link
Contributor Author

gigago commented Dec 16, 2016

Taxonomy is unrelated, don't know why it shows up here. I tried to update the branch, which appears to have resolved the conflicts.

@rhukster
Copy link
Member

Hmm. you appear to have pushed files with merge conflicts. Take a look at the "Files changed" tab.

@gigago
Copy link
Contributor Author

gigago commented Dec 17, 2016

Seems to be ok now. I will take another hard look at my workflow.

@flaviocopes
Copy link
Contributor

flaviocopes commented Dec 17, 2016

Usage:

{% if ishuman() %}
human
{% else %}
not human
{% endif %}

Avoid instantiating Browser as it's already added to the Grav object:

    public function isHuman()
    {
        return $this->grav['browser']->isHuman();
    }

But, since the same functionality can be achieved using

{% if browser.ishuman %}
human
{% else %}
not human
{% endif %}

I'm a bit unsure if it's worth this shortcut.

@gigago
Copy link
Contributor Author

gigago commented Dec 17, 2016

Sounds reasonable to me. Frankly, I didn't realize it could be solved this way. Since this is functionally equivalent and trivial to code, I'll take no for an answer.

@rhukster
Copy link
Member

Yah I think that using browser.isHuman is already simple enough. Probably just need better documentation.

I've added this line item to: getgrav/grav-learn#356

@rhukster rhukster closed this Dec 17, 2016
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.

4 participants