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

Upgrade jiathis share thirdpard plugin code template. #1796

Merged
merged 6 commits into from
Aug 9, 2017

Conversation

elkan1788
Copy link
Contributor

Upgrade jiathis share thirdpart plugin code template.

jiathis-share.png

Copy link
Collaborator

@ivan-nginx ivan-nginx left a comment

Choose a reason for hiding this comment

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

Language must not be hardcoded.

<a class="jiathis_button_qzone">QQ空间</a>
<a class="jiathis_button_tqq">腾讯微薄</a>
<a class="jiathis_button_douban">豆瓣</a>
<a class="jiathis_button_share">一键分享</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can u translate this all labels in language dir?

<a class="jiathis_button_douban">豆瓣</a>
<a class="jiathis_button_share">一键分享</a>

<a href="http://www.jiathis.com/share?uid=2140465" class="jiathis jiathis_txt jiathis_separator jtico jtico_jiathis" target="_blank">更多</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

And this too.

@elkan1788
Copy link
Contributor Author

elkan1788 commented Aug 2, 2017

@ivan-nginx Thanks for your CR. But I don't think we need do this. This third party plugin only useful in China may not other language people. Also other share target will use in EN language. So is you still hold to your point?

@elkan1788
Copy link
Contributor Author

Upgrade the analytics script code position, because someone maybe use icons display for analytics flag. So the script not suit in head mark.

Copy link
Collaborator

@ivan-nginx ivan-nginx left a comment

Choose a reason for hiding this comment

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

If u don't want to translate this, can u please add some comments about this is for China only?

@@ -44,7 +43,7 @@
<footer id="footer" class="footer">
<div class="footer-inner">
{% include '_partials/footer.swig' %}
{% include '_third-party/analytics/busuanzi-counter.swig' %}
{% include '_third-party/analytics/index.swig' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

busuanzi-counter is visible widget for footer. No need to do this.

@@ -1,3 +1,4 @@
{% include 'busuanzi-counter.swig' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

busuanzi-counter is visible widget for footer. No need to do this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, if u move busuanzi-counter index to widget, u must delete it from index, right?

@elkan1788
Copy link
Contributor Author

@ivan-nginx Sorry, maybe there you can't understand my explain about 56a8d62. There not only busuanzi-counter is visible widget, also other analytics script will support visible icons or lable, just like Baidu or CNZZ. You can visit my blog http://www.xn--7qv19ae78e.cn and scroll to footer, and I think you will known what I say, thanks.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Aug 4, 2017

@elkan1788 awesome site! China have own cyrrillic domains too, this is good.
About pull, mb i don't understand something, but as is see, u modify _third-party/analytics/index.swig file. This file include many analytics widgets, like google. And there are at most invinsible, without any widgets. Let's see:

{% include 'facebook-sdk.swig' %}
{% include 'vkontakte-api.swig' %}
{% include 'google-analytics.swig' %}
{% include 'baidu-analytics.swig' %}
{% include 'tencent-analytics.swig' %}
{% include 'tencent-mta.swig' %}
{% include 'cnzz-analytics.swig' %}
{% include 'application-insights.swig' %}
  1. FB is integrated as API. Like and comments post is supported.
  2. VK same as FB.
  3. GA does not have any widgets.
  4. Baidu have visible widget, ok.
  5. Tencent same as GA, i think.
  6. Tencent MTA have visible widget, i think.
  7. CNZZ have visible widget, ok.
  8. And AI same as GA, i think.

So, most part of this has no visible widget and if u want to see your CNZZ counter (or something else) in footer, u must do it as separated option, like busuanzi-counter.swig.

Also, u can see in history of _layout.swig file, was fix to move from body to head section for GA: 2b9e716. Pull #1705.

Please, take your time and do all rightly.

@elkan1788
Copy link
Contributor Author

@ivan-nginx Thanks for suggest and do all update under your point. Please check. :)

@@ -0,0 +1,4 @@
{% include 'busuanzi-counter.swig' %}
{% include 'busuanzi-counter.swig' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why repeat busuanzi-counter.swig?

@@ -44,7 +44,7 @@
<footer id="footer" class="footer">
<div class="footer-inner">
{% include '_partials/footer.swig' %}
{% include '_third-party/analytics/busuanzi-counter.swig' %}
{% include '_third-party/analytics/analytics-width-widget.swig' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe simple analytics-widget.swig?

@@ -1,3 +1,4 @@
{% include 'busuanzi-counter.swig' %}
{% include 'facebook-sdk.swig' %}
{% include 'vkontakte-api.swig' %}
{% include 'google-analytics.swig' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, if u move tencent, cnzz and other from index to widget, u must delete it from index, right?

@@ -0,0 +1,4 @@
{% include 'busuanzi-counter.swig' %}
{% include 'busuanzi-counter.swig' %}
{% include 'tencent-mta.swig' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are u tested tencent-mta.swig? I'm not sure, what is visible widget. Please, test it to be sure and also need to test tencent.swig.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Aug 4, 2017

@elkan1788 new comments added. Please, test it all (especially both tencent-mta) to be sure all work's ok before adding/changing pull request.

@elkan1788
Copy link
Contributor Author

@ivan-nginx So sorry about that. Now fixed the bugs and trim all analytics scripts: make sure the Tencent analytics script support widget find out in office website.

@ivan-nginx
Copy link
Collaborator

@elkan1788 seems good. Good job and thanks!

@ivan-nginx ivan-nginx merged commit 69159c8 into iissnan:master Aug 9, 2017
@ivan-nginx ivan-nginx added this to the v5.1.3 milestone Aug 23, 2017
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.

5 participants