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

More flexible margin option #23

Closed
tuelsch opened this issue Oct 10, 2017 · 6 comments

Comments

@tuelsch
Copy link
Contributor

commented Oct 10, 2017

Would be nice to have a CSS like margin option:
Single value integer: set equal pixel margins (24) => margin: 24px;
Single value string: directly set margin ("3rem auto") => margin: 3rem auto;

This would give the option to leverage other CSS units and define different margins on each side.

@francoischalifour

This comment has been minimized.

Copy link
Owner

commented Oct 12, 2017

I thought about it but wondered the actual use case. Could you share yours?

A string would indeed be more flexible but not trivial to implement since the margin is computed in JavaScript. I find it a bit silly to add a CSS parser just for this.

Thanks for requesting though. Share your use case so I can study this more in depth.

@tuelsch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2017

I thought it was nice to have a close button if an image was zoomed. I wanted to make sure the button does not overlap the image and at the same time display the image as large as possible. So it would have been nice to define a top margin of about 72px and side/bottom margins of 24px.

I saw that the margins were calculated after I posted this, so maybe an array approach would be fitting, leaving out other units and auto:

  • Int/Array single item: (24, [24]) => margin: 24px;
  • Array, two items: ([24, 50]) => margin: 24px 50px;
  • Array, three items: ([1,2,3]) => margin: 1px 2px 3px 2px;
  • Array, four items: ([1,2,3,4]) => margin: 1px 2px 3px 4px;

What do you think?

@francoischalifour

This comment has been minimized.

Copy link
Owner

commented Oct 12, 2017

Making it a string or an array is basically the same, so a string would be more CSS-ish. My concern was more about em and rem units.

Anyway, the whole point of the library is to stick to Medium's design. I don't think this use case is enough for me to implement this feature for now, sorry! If you have time though, feel free to give it a try. I could review the API design and guide you through it.

@francoischalifour

This comment has been minimized.

Copy link
Owner

commented Oct 24, 2017

Hi @tuelsch! I've been thinking about your issue and started seeing it as a much higher level today. I think I'm onto something.

I don't think your problem is about margins, it's about templating. You'd essentially like to not only render the image, but some components alongside it.

I thought about using a container option that you would pass to mediumZoom. This container would be a template element that you can feed with your needs. It's up to you to style it - you could do whatever you want really. The image zooms and fits to the [data-zoom-container] element.

Here is an example, reproducing the new Dropbox Paper zoom:

<template id="paper-zoom">
  <div>
    <section>
      <header>
        &times;
      </header>
      <div data-zoom-container></div>
    </section>
    <aside>
      <h3>Comments</h3>
      <p>...</p>
    </aside>
  </div>
</template>

<img
  src="image.jpg"
  alt="Zoomable image"
>

<script>
  mediumZoom('img', {
    container: '#zoom-paper',
    background: 'rgba(247,249,250,0.97)',
    margin: 24
  })
</script>

medium-zoom-paper

Another example reproducing Facebook style:

medium-zoom-facebook

I've just started working on this and it might take some time to figure out a good API.

What's your thoughts?

Edit: added Facebook example

@francoischalifour

This comment has been minimized.

Copy link
Owner

commented Oct 27, 2017

The next release will include containers and templates support. You can achieve what you want to do with:

mediumZoom('img', {
  container: {
    top: 64
  }
})

Stay tuned!

@tuelsch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2017

This takes the idea a lot further than I originally thought - which is absolutely great. Thank you for your very good work! Having templates and containers allows for a lot more flexibility and in my opinion is more future proof. I'd try to put some work into the library for the next time I see an opportunity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.