Skip to content

Image support#97

Closed
ngscheurich wants to merge 2 commits intoxou:masterfrom
newaperio:image-support
Closed

Image support#97
ngscheurich wants to merge 2 commits intoxou:masterfrom
newaperio:image-support

Conversation

@ngscheurich
Copy link

@ngscheurich ngscheurich commented Mar 26, 2020

This adds support for embedding images in workbooks. It technically does
two main things:

  • Implements the changes introduced by proactively's image support
    branch
    on top of the most recent commit to master
  • Adds width and height options to Elixlsx.Image so that it's
    possible to configure how many columns/rows an image should occupy

All the credit for this goes to proactively.


Note: This PR duplicates most of the work in #79. I needed a working copy of this ASAP for a project, so I created this branch. Feel free to close this in favor of #79 if work is still being done there!

This adds support for embedding images in workbooks. It technically does
two main things:

- Implements the changes introduced by [proactively's image support
  branch](https://github.com/proactively/elixlsx/tree/image-support-no-reformat)
  on top of the most recent commit to master
- Adds `width` and `height` options to `Elixlsx.Image` so that it's
  possible to configure how many columns/rows and image should occupy
Copy link
Owner

@xou xou left a comment

Choose a reason for hiding this comment

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

Awesome, thank you so much! Just a few nitpicks :)

Thanks!

Comment on lines +525 to +526
<a:off x="1812547" y="3503925"/>
<a:ext cx="240431" cy="237600"/>
Copy link
Owner

Choose a reason for hiding this comment

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

These numbers seem oddly specific. Can you add a comment where they come from?


sheet7 =
sheet7
|> Sheet.insert_image(0, 5, "ladybug-3475779_640.jpg")
Copy link
Owner

Choose a reason for hiding this comment

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

Can you verify that this is a picture we can actually use in an open source project?
If required, can you add an attribution notice (not a legal expert, but maybe just a ladybug-3475779_640.jpg.NOTICE txt file or something)


cond do
length(sheet.rows) <= rowidx ->
# append new rows, call self again with new sheet
Copy link
Owner

Choose a reason for hiding this comment

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

The below code is now mostly handled by maybe_extend / dead, if I'm not mistaken?

@proactively
Copy link

Thank you so much for picking this up. The photo is a Free for commercial use
No attribution required photo: https://pixabay.com/photos/ladybug-insect-beetle-nature-3475779/

@ngscheurich
Copy link
Author

@proactively My pleasure. My company needed this functionality ASAP for a client app, so it was great to find that most of the work had been done! I'll try and take a look at this and get it shaped up in the near future.

@grrrisu
Copy link

grrrisu commented Nov 2, 2020

@ngscheurich @xou Any news on this PR? Would be an awesome feature.

@frerich
Copy link

frerich commented Jun 23, 2021

For what it's worth, I'd love to have a release with this feature :-)

@sergey-chechaev
Copy link

Do you have any updates on this PR?

@ngscheurich
Copy link
Author

Closing this in favor of #124. Thanks for your work, @ustrajunior!

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.

6 participants