Skip to content
This repository has been archived by the owner on May 22, 2020. It is now read-only.

Fix aspect ratio issues, add max-width/max-height options. #274

Merged
merged 3 commits into from
Oct 15, 2017

Conversation

RafaPolit
Copy link
Contributor

@RafaPolit RafaPolit commented Oct 13, 2017

Current lightbox implementation has some issues with aspect ratio and margins:

Current buggy behaviors

  • Modal imposes a 15px padding right to accommodate the scrollbar. Since this is a no-issue if you are going to force the size to never scroll, this is not required.
  • Aspect ratio (when limiting size vertically) is working from a wrong assumption: that aspect ratio of OUTER container should be consistent. This apparently works on larger sizes, but as modal gets smaller, its clear this is the wrong approach: the INNER aspect ratio should match, and it changes having fixed-sizes borders.
  • Above 575px the modal has no margins and can touch the borders of the screen, but below that value, a 10px margin is added by bootstrap.

Some missing features

  • Modal is not centered on the screen, for image showcase, this is preferable than the normal behavior.
  • There is no way to limit the modal to a maximum size. Which could be desirable on scenarios where you have retina-optimized images (2x) that have twice the size as the final desired screen display. (I reported this as issue Add max-width/height options #273 )

A brief example of the issues:
lightbox-current

Notice the higher padding on the bottom and the extra margin on the right.

Proposed fixes

  • Modal overrides the 15px padding always to 0.
  • Aspect ratio is calculated for the inner space
  • Modal always has a 10px separation from the borders
  • Modal is centered in the window (taken from Fix css centering on mobile #268)
  • You can specify a max-width and / or max-height optionally

lightbox-pullrequest

I have added further example to the index.html examples file.

Forced 10px margin when window size > 575px to keep a congruent display.
Removed 15px margin for scroll.
</div>
</div>

<h3 id="single-image">Limit Image Size</h3>
Copy link
Owner

Choose a reason for hiding this comment

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

The id here is wrong, but I will change for the gh-pages branch

@@ -1,7 +1,7 @@
{
"name": "ekko-lightbox",
"description": "A lightbox gallery plugin for Bootstrap based on the modal plugin",
"version": "5.2.0",
Copy link
Owner

Choose a reason for hiding this comment

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

I'll bump this to 5.3.0 before publishing to npm as there's some significant feature changes here

@ashleydw
Copy link
Owner

Thanks! :)

@ashleydw ashleydw merged commit e445452 into ashleydw:master Oct 15, 2017
@ashleydw
Copy link
Owner

5.3.0 has been released. When cdnjs updated, I'll update the docs pages 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants