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

Allow single link galleries #83

Merged
merged 4 commits into from
Jun 12, 2016
Merged

Allow single link galleries #83

merged 4 commits into from
Jun 12, 2016

Conversation

LorenzCK
Copy link
Contributor

When the gallery selector points to single <A> elements, the elements themselves are used as lightbox links instead of their <A> descendants.

Rationale

On pages with multiple stand-alone images, which are not part of a gallery and are not enclosed in a wrapping gallery tag, it can make sense for users to setup the lightbox individually for each link.

For instance, in:

<p>Page body.</p>
<p><a href="img.jpg" class="lightbox"><img src="img.jpg" /></a></p>
<p>More <a href="img.jpg" class="lightbox">images</a>.</p>

it can be difficult to enable the lightbox on both images. With the changes in this pull request, the <A> elements can be directly used to create single-image galleries:

baguetteBox.run('a.lightbox');

Nota bene

These changes make it impossible to create galleries enclosed by an <A> tag. For instance:

<a href="#" class="gallery">
<a href="img1.jpg"><img src="img1.jpg" /></a>
<a href="img2.jpg"><img src="img2.jpg" /></a>
</a>

In this case the .gallery selector couldn't be used anymore to setup a gallery. However, since enclosing a gallery in an anchor tag should be highly unlikely, these changes probably wouldn't impact any real existing code.

When the gallery selector points to <A> elements, the elements themselves are used as lightbox links (instead of their <A> descendants).
Allows users to setup heterogeneous galleries where elements are not contained by a single element.
@XhmikosR
Copy link
Contributor

Not bad, but I believe #81 will be more flexible.

@@ -149,8 +149,14 @@
[].forEach.call(gallery, function(galleryElement) {
if(userOptions && userOptions.filter)
regex = userOptions.filter;

if(galleryElement.tagName === 'A')
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs formatting, brackets, spaces, etc.

Also, you shouldn't declare the var like that; it can be misleading. Better declare it outside and set its value then.

@LorenzCK
Copy link
Contributor Author

@XhmikosR: #81 looks very promising. However, I would argue that this addition allows to create simple galleries without having to change the existing markup of the page (if you have no control over the markup or if you have a large number of pages that would need to be changed).
In fact I made this little change for a project I was working on, where I had large numbers of <a> tags embedded in too many pages to be edited quickly.

Fixed formatting and brackets as rightly noted in your previous comment.

@@ -159,8 +159,13 @@
if (userOptions && userOptions.filter) {
regex = userOptions.filter;
}
var tags = [];
if(galleryElement.tagName === 'A') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space after if.

@XhmikosR
Copy link
Contributor

I see both points, don't get me wrong. I believe a combination of both would be the best.

@XhmikosR
Copy link
Contributor

Doesn't this conflict with #101?

@XhmikosR
Copy link
Contributor

Ping @feimosi

@feimosi
Copy link
Owner

feimosi commented Apr 30, 2016

@XhmikosR it shouldn't conflict with each other, these are two different areas of the script, unless I don't notice something.

I think that's a good solution for now and with the v2.0 I'll make #81 possible,
@LorenzCK could you make a rebase before merging?

@feimosi
Copy link
Owner

feimosi commented May 27, 2016

Hi @LorenzCK, quite a little bit has changed in the codebase lately, could you take a look and rebase your branch?

@LorenzCK
Copy link
Contributor Author

Hi @feimosi, sure! Will look into it by tomorrow. Thanks.

Conflicts:
	src/baguetteBox.js
selectorData.galleries.push(gallery);


var gallery = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@feimosi @XhmikosR The order here didn't look right to me, IMHO. I re-ordered the gallery declaration and moved the push call after the forEach below. Hope this is right.

Copy link
Owner

Choose a reason for hiding this comment

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

You're right, it's much cleaner this way 👍

@feimosi feimosi merged commit 11f18b2 into feimosi:master Jun 12, 2016
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.

3 participants