-
-
Notifications
You must be signed in to change notification settings - Fork 641
Parse image dimensions #445
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
base: master
Are you sure you want to change the base?
Conversation
| return combineResults(blips.map(readBlip.bind(null, element))); | ||
| var dimensions = | ||
| element.first('wp:extent') || | ||
| picture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you implement safe nested property access in .getElementsByTagName()? If not, why aren't we checking if the referenced tags exist?
| .attributes['a:ext'] | ||
|
|
||
| return combineResults(blips.map((blip) => { | ||
| return readBlip(element, blip, dimensions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unwrapping the map function because there's no benefit to keeping it inlined. It only confuses the argument order.
| return readImage(blipImageFile, altText); | ||
| return readImage(blipImageFile, { | ||
| altText, | ||
| height: dimensions.attributes['cy'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Image dimensions are defined on a higher node than blips, so I assume dimensions apply to all blips. Let me know if I'm wrong in this assumption.
| .getElementsByTagName("a:blip"); | ||
|
|
||
| return combineResults(blips.map(readBlip.bind(null, element))); | ||
| var dimensions = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some vendors (Microsoft, Google Docs) store image dimensions in wp:extent while others, like Apple Pages, set the on the pic:pic node instead. We have to parse both scenarios (they describe the same thing and are mutually exclusive).
|
|
||
| function readDrawingElement(element) { | ||
| var blips = element | ||
| var picture = element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm lifting up the picture node reference so we don't have to look it up multiple times. If this is premature, let me know.
Motivation
This change parses out the image's
cxandcyvalues that describe the end image dimensions in the document. As I've mentioned in the issue, these values are end values (e.g. after image resizing). DOCX doesn't store the original image dimensions. If consumers need those, they would have to infer them from the original image buffer using tools likesharp. Most of the time, however, you don't care about the original dimensions since you parse the document to render it in some way.Roadmap
cxandcy? They are in EMU by default. We can convert them to pixels, but we have to be consistent with how Mammoth treats other measurements here.