-
Notifications
You must be signed in to change notification settings - Fork 326
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
Automatic Brew thumbnail generation option #3219
Conversation
Remove 5ePHB assets and fonts.
This reverts commit 27d53f2.
There are two bugs that may be resolved by being on HTTPS
Evaluators, please test with an assortment of complicated brews? I don't have a good library built up yet. |
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.
It seems like the generated image's "frame" is set by the brewRenderer.
Note that I have the brewRenderer sized to just fit the page.
This is true even with a default page, without custom styling. It seems likely that the capture is including the node's margin
? So with a wide brewRenderer, the margin is larger, and the captured image is very wide (and thus not fitting in the thumbnail box).
Edit: You can change the css of the captured node before the capture. I left a comment on that line.
There are a lot of cross-origin errors in the console when using this tool, all related to issues getting external stylesheets from fontawesome, googlefonts, etc.
|
||
ThumbnailCapture : async function() { | ||
const bR = parent.document.getElementById('BrewRenderer'); | ||
const brewRenderer = bR.contentDocument || bR.contentWindow.document; |
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.
Curious why this is an OR
between contentDocument
and contentWindow.document
? From what I can tell, it is the same thing?
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.
My memory of the reference code is that it should be the same but for some browser situations, it may not be.
@@ -333,6 +350,9 @@ const MetadataEditor = createClass({ | |||
<button className='display' onClick={this.toggleThumbnailDisplay}> | |||
<i className={`fas fa-caret-${this.state.showThumbnail ? 'right' : 'left'}`} /> | |||
</button> | |||
<button className='display' onClick={this.ThumbnailCapture}> | |||
<i className={`fas fa-image`} /> |
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 think that switching to a camera icon is something to consider?
<i class="fas fa-camera"></i>
Maybe the fa-image
icon is more indicative of "open from image library" rather than "generate new picture". Further, even though none of the other fields/controls currently have one, maybe this should have a tooltip?
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.
This is an excellent idea. I will do that.
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.
Added a comment about removing the page margin before capture.
(sorry for the "second" review, still getting the hang of them and didn't realize you couldn't append to an existing review)
const topPage = brewRenderer.getElementsByClassName('page')[0]; | ||
const props = this.props; | ||
|
||
htmlimg.toPng(topPage, { preferredFontFormat: 'woff2' }).then(function(dataURL){ |
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.
Remove the .page
margin before doing the capture:
htmlimg.toPng(topPage, { preferredFontFormat: 'woff2', style: { margin: 'unset' } }).then(function(dataURL){
Yeah, that's been a problem for me here and I believe is at least one factor on the PDF generation. My thinking was that while imperfect it should handle the majority of coverpages. |
I'm going to push this back to draft until I can sort out a way to deal with the Cross-site issues. |
Add suggested changes
Am i the only one to notice the Drop cap missing? Also, we could use this with #3321 to auto generate previews? |
I'm assuming you have read https://developers.google.com/privacy-sandbox/3pcd |
Wow, tried the tool, so far:
|
Hmm. Looks like the CORS related issues have gotten worse. The only idea I've had around solving this is is to put proxy endpoint on the app and translate URLs when this function is called. |
Closing in favor of #3501. |
This is a feature addition that adds the option of creating a thumbnail directly from the first page of the brew.