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

Filter hreflang #267

Merged
merged 8 commits into from Jul 25, 2017

Conversation

2 participants
@diedexx
Contributor

diedexx commented Jul 21, 2017

This pull request fixes issue #266.

What's Included in This Pull Request

  • Two new filters
    • multilingualpress.hreflang_output_method Filters the used output methods for the hreflang links, so either the html or header output can be disabled. Both methods (html and headers) are enabled by default.
    • multilingualpress.hreflang_translations Filters the available translations before outputting their hreflang links, so the hreflang links can be altered with a single filter. For example, with this filter a 'x-default' could be added easily.
@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Jul 21, 2017

Contributor

Hi,

thanks a lot for this.

Please let's first sort things out in #266, so we know where we want to go from here, OK?

Could you also please re-open the pull request against the new branch 2.7?
I don't think I can do this from GitHub (only via command line, which would mean I will have to close this pull request at some point, and note merge it here).

Contributor

tfrommen commented Jul 21, 2017

Hi,

thanks a lot for this.

Please let's first sort things out in #266, so we know where we want to go from here, OK?

Could you also please re-open the pull request against the new branch 2.7?
I don't think I can do this from GitHub (only via command line, which would mean I will have to close this pull request at some point, and note merge it here).

@diedexx diedexx changed the base branch from 2.6 to 2.7 Jul 21, 2017

@diedexx

This comment has been minimized.

Show comment
Hide comment
@diedexx

diedexx Jul 21, 2017

Contributor

You can edit the base branch by editing the title of the pr (strangely enough) 😉

Contributor

diedexx commented Jul 21, 2017

You can edit the base branch by editing the title of the pr (strangely enough) 😉

@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Jul 21, 2017

Contributor

Ha! I knew that I read it somewhere that it was possible - but yes, editing the title was nothing I tried. 😀

Contributor

tfrommen commented Jul 21, 2017

Ha! I knew that I read it somewhere that it was possible - but yes, editing the title was nothing I tried. 😀

Process feedback
- use constant/flags for selecting the hreflang_output
- rename the hreflang_output filter
- remove duplicate code
@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Jul 24, 2017

Contributor

Yep, that looks much better to me.
Just one thing: could you move the constants to the Mlp_Hreflang_Header_Output class, please? Names TYPE_HTTP_HEADER and TYPE_HTML_LINK_TAG.

Thanks!

I will merge the PR after that, and adapt a few minor things (formatting etc.).

Contributor

tfrommen commented Jul 24, 2017

Yep, that looks much better to me.
Just one thing: could you move the constants to the Mlp_Hreflang_Header_Output class, please? Names TYPE_HTTP_HEADER and TYPE_HTML_LINK_TAG.

Thanks!

I will merge the PR after that, and adapt a few minor things (formatting etc.).

@tfrommen

Looks good to me.

@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Jul 25, 2017

Contributor

Did some minor changes. Works for you, @diedexx?

Thanks for your work!

Contributor

tfrommen commented Jul 25, 2017

Did some minor changes. Works for you, @diedexx?

Thanks for your work!

@diedexx

This comment has been minimized.

Show comment
Hide comment
@diedexx

diedexx Jul 25, 2017

Contributor

Works for me! 👍

Contributor

diedexx commented Jul 25, 2017

Works for me! 👍

@tfrommen tfrommen merged commit dd8769e into inpsyde:2.7 Jul 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment