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

(feat): Add JS suport for instagram gallery #560

Closed
wants to merge 1 commit into from

Conversation

iranzo
Copy link
Member

@iranzo iranzo commented Jan 27, 2020

Prerequisites

Recommended Steps

  • My patch adds a new feature, therefore I have also added a help article about it
  • My patch changes Elegant behavior, therefore I have updated the help article to reflect this change
  • My commits are signed

Description

This PR modifies article.html so that it can show instagram pictures or group of pictures as galleries via adding a new yaml preamble named 'instagram'

Example at: https://deploy-preview-560--pelicanelegant.netlify.com/how-to-use-galleriaio-plugin

@iranzo
Copy link
Member Author

iranzo commented Jan 27, 2020

Related with #385

@iranzo iranzo force-pushed the instagram branch 2 times, most recently from d255afd to 3610529 Compare January 27, 2020 16:46
@iranzo iranzo force-pushed the instagram branch 8 times, most recently from fa02db9 to c550204 Compare January 28, 2020 12:57
@iranzo iranzo requested a review from talha131 January 29, 2020 22:10
@talha131 talha131 self-assigned this Jan 30, 2020
Copy link
Member

@talha131 talha131 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR. I have requested some changes from you.

documentation/content/Supported Plugins/galleriaio.md Outdated Show resolved Hide resolved
templates/article.html Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@

{% block meta_tags_in_head %}
{% if article.redirect %}
<meta http-equiv="refresh" content="0;URL={{ article.redirect}}" />
<meta http-equiv="refresh" content="0;URL={{ article.redirect }}" />
Copy link
Member

Choose a reason for hiding this comment

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

Please rebase.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

templates/article.html Outdated Show resolved Hide resolved
documentation/content/Supported Plugins/galleriaio.md Outdated Show resolved Hide resolved
@talha131
Copy link
Member

@iranzo Kindly fix the issues I have mentioned.

Next, this patch adds one more dependency to the jquery. We can use other JS frameworks that do not depend on jquery, like

  1. https://photoswipe.com/
  2. https://sachinchoolur.github.io/

Will you be able to replace galleriajs with photoswipe? If not then we both can work together on this feature. It ain't difficult. We can finish it in two days.

Signed-off-by: Pablo Iranzo Gómez <Pablo.Iranzo@gmail.com>
@iranzo
Copy link
Member Author

iranzo commented Jan 30, 2020

@iranzo Kindly fix the issues I have mentioned.

Next, this patch adds one more dependency to the jquery. We can use other JS frameworks that do not depend on jquery, like

  1. https://photoswipe.com/
  2. https://sachinchoolur.github.io/

Will you be able to replace galleriajs with photoswipe? If not then we both can work together on this feature. It ain't difficult. We can finish it in two days.

All other things addressed.
2nd url doesn't load, for 1st one it looks nice, but to get this kind of automation (just instragram_id) I will need to check if it's possible and how.

If we get both supported, we can make something that is compatible with both so that end user can decide or us put default of our preferred one and deprecate the other (if we want to get this included asap)

@talha131
Copy link
Member

2nd url doesn't load

Sorry. Here you go https://sachinchoolur.github.io/lightgallery.js/

If we get both supported, we can make something that is compatible with both so that end user can decide or us put default of our preferred one and deprecate the other (if we want to get this included asap)

Too much work. I say as we have already decided to drop jquery, we should only choose a solution that works without jquery.

but to get this kind of automation (just instragram_id) I will need to check if it's possible and how.

It won't be much difficult to refactor jquery code to plain JS and make it work with photoswipe. As I said, we can work together on this feature.


Meanwhile, do fix the issues in this PR that I have pointed out. Photoswipe change will be made on top of this PR.

@iranzo
Copy link
Member Author

iranzo commented Jan 30, 2020

One concern on ligthtgallery:

image

Is listed as free and open source library and repo contains: https://github.com/sachinchoolur/lightgallery.js/blob/master/LICENSE.md so it should be ok for us to use it

@talha131
Copy link
Member

Right. I noticed the license.

Can you please compare both the libraries and let me know which one you think is better suited for us?
Like development activity, community etc.

@iranzo
Copy link
Member Author

iranzo commented Jan 30, 2020

Based on latest posts and amount of contributors, photoswipe seems the most maintained (lightgallery is 1 year back in time and 10 contribs vs 42 in photoswipe)

@talha131
Copy link
Member

Ok so photoswipe it is.

Now, if you can do this task alone then please update the PR.
Else if you need my help then,

  1. Resolve the issues I pointed out in the review
  2. I will open a new branch so that we both can work together on it.

@iranzo
Copy link
Member Author

iranzo commented Jan 30, 2020

@talha131 the ones about galleria.io are already in this PR and in the preview (together with admonition usage for the single-multiple)

@talha131
Copy link
Member

Thank you @iranzo.

You can view my progress and changes here. #567

To make or add your changes open your PR against feat-image-gallery branch.


Closing this PR in favor of #567

@talha131 talha131 closed this Jan 30, 2020
@iranzo iranzo deleted the instagram branch September 10, 2021 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants