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

Add share.js support #1095

Closed
wants to merge 3 commits into from
Closed

Add share.js support #1095

wants to merge 3 commits into from

Conversation

GetToSet
Copy link

@GetToSet GetToSet commented Aug 15, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines.
  • Tests for the changes was maked (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs in NexT website have been added / updated (for features).

PR Type

What kind of change does this PR introduce?

  • Bugfix.
  • Feature.
  • Code style update (formatting, local variables).
  • Refactoring (no functional changes, no api changes).
  • Build related changes.
  • CI related changes.
  • Documentation content changes.
  • Other... Please describe:

What is the current behavior?

Added support for share.js.

Issue resolved: N/A

What is the new behavior?

  • Screenshots with this changes:

屏幕快照 2019-08-15 21 43 28

  • Link to demo site with this changes: This feature will be used on my own site, which is still under construction.

How to use?

In NexT _config.yml:

# share.js
# See: https://github.com/overtrue/share.js
# networks: weibo,qq,wechat,tencent,douban,qzone,linkedin,diandian,facebook,twitter,google
sharejs:
  enable: true
  #networks: weibo,qq,wechat,tencent,douban,qzone,linkedin,diandian,facebook,twitter,google
  wechat_qrcode:
    title: share.title
    prompt: share.prompt

Does this PR introduce a breaking change?

  • Yes.
  • No.

@welcome
Copy link

welcome bot commented Aug 15, 2019

Thanks so much for opening your first PR here!

@CLAassistant
Copy link

CLAassistant commented Aug 15, 2019

CLA assistant check
All committers have signed the CLA.

@GetToSet GetToSet changed the title Added share.js support. feat | Added share.js support. Aug 15, 2019
@GetToSet GetToSet changed the title feat | Added share.js support. Added share.js support. Aug 15, 2019
layout/_partials/share/sharejs.swig Outdated Show resolved Hide resolved
layout/_partials/share/sharejs.swig Outdated Show resolved Hide resolved
_config.yml Outdated Show resolved Hide resolved
@stevenjoezhang
Copy link
Contributor

stevenjoezhang commented Aug 15, 2019

Thanks. Let's wait for jsdelivr to update this package 😂 https://www.jsdelivr.com/package/npm/social-share.js

jsDelivr
A free, fast, and reliable Open Source CDN for npm and GitHub with the largest network and best performance, perfectly suited for production use.

layout/_partials/share/sharejs.swig Show resolved Hide resolved
layout/_partials/share/sharejs.swig Show resolved Hide resolved
layout/_partials/share/sharejs.swig Show resolved Hide resolved
@1v9 1v9 added this to the v7.4.0 milestone Aug 15, 2019
@@ -1,6 +1,7 @@
.post-widgets {
border-top: 1px solid #eee;
margin-top: 45px;
padding-top: 20px;
Copy link
Member

Choose a reason for hiding this comment

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

this change for what?

Copy link
Author

Choose a reason for hiding this comment

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

Share.js doesn't include a top padding and share components seems to be too close to the divider.
屏幕快照 2019-08-16 06 58 57

Copy link
Member

Choose a reason for hiding this comment

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

OK, would you commit much more? I sae you mark resolved upstairs

Copy link
Contributor

Choose a reason for hiding this comment

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

屏幕快照 2019-08-16 上午9 50 10

Move this padding-top: 20px; somewhere else. It breaks the style of all other widgets.

{%- if theme.likely.enable or (theme.needmoreshare2.enable and theme.needmoreshare2.postbottom.enable) %}
<div class="social-share">
{%- if theme.likely.enable or (theme.needmoreshare2.enable and theme.needmoreshare2.postbottom.enable) or theme.sharejs.enable %}
<div class="post-social-share">
Copy link
Member

Choose a reason for hiding this comment

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

No additional changes, add extra judgment for sharejs

Copy link
Member

Choose a reason for hiding this comment

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

{%- if theme.sharejs.enable %}
  <div class="social-share" data-sites="{{ theme.sharejs.networks }}" data-wechat-qrcode-title="{{ __(theme.sharejs.wechat_qrcode.title) }}" data-wechat-qrcode-helper="{{ __(theme.sharejs.wechat_qrcode.prompt) }}"></div>
{%- endif %}

Or you can put options to front-end, we have global CONFIG, check head.swig.

Copy link
Author

@GetToSet GetToSet Aug 15, 2019

Choose a reason for hiding this comment

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

I changed css class social-share to post-social-share because share.js has hard-coded selector for social-share, which is exactly the same and will definitely result in problems if multiple share components are enabled.

Copy link
Author

Choose a reason for hiding this comment

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

I performed a global search to make sure that no css code selects for social-share, I think it's necessary to introduce this change.

Copy link
Author

Choose a reason for hiding this comment

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

{%- if theme.sharejs.enable %}
  <div class="social-share" data-sites="{{ theme.sharejs.networks }}" data-wechat-qrcode-title="{{ __(theme.sharejs.wechat_qrcode.title) }}" data-wechat-qrcode-helper="{{ __(theme.sharejs.wechat_qrcode.prompt) }}"></div>
{%- endif %}

Or you can put options to front-end, we have global CONFIG, check head.swig.

Is it suitable to put configuration scripts to _partials/share/sharejs.swig? Code may be injected to all pages even if no share components present, or should I put it under _partials/post/post_footer.swig?

Copy link
Member

Choose a reason for hiding this comment

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

Forget this, you can check likely, css/html/js part is independent

Copy link
Member

@1v9 1v9 Aug 15, 2019

Choose a reason for hiding this comment

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

For now, keep these share services unified, If you have more ideas, open another PR when this merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I performed a global search to make sure that no css code selects for social-share, I think it's necessary to introduce this change.

.social-share will be blocked by adblock, but .post-social-share won't.

@1v9 1v9 changed the title Added share.js support. Add share.js support Aug 15, 2019
@stevenjoezhang
Copy link
Contributor

stevenjoezhang commented Aug 16, 2019

How about using jsdelivr as default CDN: https://cdn.jsdelivr.net/npm/social-share.js@1/dist/js/social-share.min.js

@sli1989
Copy link
Collaborator

sli1989 commented Aug 16, 2019

How about using jsdelivr as default CDN: https://cdn.jsdelivr.net/npm/social-share.js@1/dist/js/social-share.min.js

can the version number of all cdn verder be replaced by @latest

@stevenjoezhang
Copy link
Contributor

stevenjoezhang commented Aug 16, 2019

Besides, social-share.js was last published 3 years ago: https://www.npmjs.com/package/social-share.js

This is a potential problem. Gitment and Han have not been updated for more than a year, they have brought a lot of issues and finally be removed from NexT.

npm
create social share buttons on your site.

@stevenjoezhang
Copy link
Contributor

@stevenjoezhang
Copy link
Contributor

stevenjoezhang commented Aug 16, 2019

My suggestion is to maintain a fork of share.js by NexT. Thus we can remove the hard-coded selector for .social-share and add a padding-top in the stylesheet

@stevenjoezhang
Copy link
Contributor

stevenjoezhang commented Aug 17, 2019

Thanks for your contributing. New plugin created: https://github.com/theme-next/hexo-next-share

GitHub
Content Sharing Services for NexT. Contribute to theme-next/hexo-next-share development by creating an account on GitHub.

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