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

Fix fancybox & medium-zoom for hexo asset_img tag #1166

Merged
merged 2 commits into from
Sep 19, 2019
Merged

Fix fancybox & medium-zoom for hexo asset_img tag #1166

merged 2 commits into from
Sep 19, 2019

Conversation

sankoshine
Copy link
Contributor

@sankoshine sankoshine commented Sep 17, 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).
    • Mist have been tested.
    • Pisces 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?

Issue resolved: N/A

If I use hexo tag plugins to insert an image, it will not be rendered as medium zoom style.

i.e. {% asset_img 2222.jpg %}

What is the new behavior?

Medium zoom take effects on images whether imported with hexo asset_img tag or markdown-style ![](image.jpg).

  • Screenshots with this changes: N/A
  • Link to demo site with this changes: N/A

How to use?

Just set true.

In NexT _config.yml:

mediumzoom: true

Does this PR introduce a breaking change?

  • Yes.
  • No.

@welcome
Copy link

welcome bot commented Sep 17, 2019

Thanks so much for opening your first PR here!

@CLAassistant
Copy link

CLAassistant commented Sep 17, 2019

CLA assistant check
All committers have signed the CLA.

@jiangtj
Copy link
Member

jiangtj commented Sep 17, 2019

Can you test whether <img> under <a> label is also managed by mediumzoom ?

Some users expect the link to work.

@stevenjoezhang
Copy link
Contributor

And you can also fix fancybox in this PR

@sankoshine
Copy link
Contributor Author

sankoshine commented Sep 17, 2019

Can you test whether under label is also managed by mediumzoom ?

Some users expect the link to work.

Yes I have tested all following:

  • {% asset_img a.jpg %}
  • <img src="a.jpg" />
  • ![](a.jpg)
  • [{% asset_img a.jpg %}](https://site.com)
  • <a href="https://site.com">{% asset_img 2222.jpg %}</a>
  • [![](a.jpg)](https://site.com)

Each is working fine. MediumZoom on images with link won't be triggered.

@1v9 1v9 added this to the v7.4.1 milestone Sep 17, 2019
@stevenjoezhang stevenjoezhang changed the title Fix medium zoom for images generated by hexo tag plugins Fix fancybox & medium zoom for hexo asset_img tag Sep 17, 2019
@1v9 1v9 changed the title Fix fancybox & medium zoom for hexo asset_img tag Fix fancybox & medium-zoom for hexo asset_img tag Sep 17, 2019
Copy link
Contributor

@stevenjoezhang stevenjoezhang left a comment

Choose a reason for hiding this comment

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

In fact, in the stylesheet of theme-next, .post-body > p > img is different from .post-body > img (with a margin of 20px)
Thus a better solution is to ask Hexo to fix the bug of asset_image tag: <img> should always be wrapped with <p>.

@jiangtj
Copy link
Member

jiangtj commented Sep 18, 2019

Thus a better solution is to ask Hexo to fix the bug of asset_image tag: <img> should always be wrapped with <p>.

This may be marked rendering problems, markdown-it is not the same with marked results.

image
image

@jiangtj
Copy link
Member

jiangtj commented Sep 18, 2019

Strange, the official demo normal, hexo-render-markd also unmodified {%%}

image

Copy link
Contributor

@stevenjoezhang stevenjoezhang left a comment

Choose a reason for hiding this comment

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

There seems to be no more elegant solution

@stevenjoezhang stevenjoezhang merged commit 006a4b0 into theme-next:master Sep 19, 2019
@welcome
Copy link

welcome bot commented Sep 19, 2019

Congrats on merging your first pull request here! 🎉 How awesome!

@1v9
Copy link
Member

1v9 commented Sep 20, 2019

@all-contributors please add @sankoshine for bug

@allcontributors
Copy link
Contributor

@1v9

I've put up a pull request to add @sankoshine! 🎉

tongluyang pushed a commit to tongluyang/hexo-theme-next that referenced this pull request Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants