Skip to content
This repository has been archived by the owner on Aug 24, 2021. It is now read-only.

Enable image popup on mobile device #168 #200

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enable image popup on mobile device #168 #200

wants to merge 1 commit into from

Conversation

jia1
Copy link

@jia1 jia1 commented Dec 6, 2016

Fixes #168

Ready for review.

screenshot_2016-12-06-21-53-05

Popped-up images are set to fit the width of the screen.

@jia1 jia1 changed the title Add popups to images for zoom effect Enable image popup on mobile device #168 Dec 7, 2016
@damithc
Copy link
Contributor

damithc commented Dec 12, 2016

Ready for review?

@jia1
Copy link
Author

jia1 commented Dec 12, 2016

Yes. Apologies for leaving out the indication of ready to review.

@damithc
Copy link
Contributor

damithc commented Dec 13, 2016

@acjh can review this one?


$(window).load(function() {
addModalToImg();
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work if executed in $(document).ready as recommended?

padding: 0;
}

.mfp-with-zoom .mfp-container,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -257,6 +258,7 @@

</script>
<p>-------------------------------------End of document----------------------------------------</p>
<script type="text/javascript" src="../vendor/magnific-popup/jquery.magnific-popup.min.js"></script>
Copy link
Collaborator

@acjh acjh Dec 13, 2016

Choose a reason for hiding this comment

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

Currently, popups are initialised in common.js, which handbook doesn't load.
But some considerations before you rush to initialise popups in handbook.js:

  • Cloning code is generally bad.
  • Handbook content now loads in a different manner - you may have to iron those bugs.
  • Anyway, there's no image in handbook needing popup, that's not already a good size.

I'd recommend targeting the schedule for now, which is sufficient (and what's intended).

*/

/* padding-bottom and top for image */
.mfp-no-margins img.mfp-img {
Copy link
Collaborator

Choose a reason for hiding this comment

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

.mfp-no-margins is an unused class.

@acjh
Copy link
Collaborator

acjh commented Dec 13, 2016

@jia1 Note that popup != zoom effect, which in the case of magnific-popup refers to animation.
Currently, there's no zoom effect (see Week 5 in my earlier attempt if unclear), but it's not required.

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

Successfully merging this pull request may close these issues.

3 participants