-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Preview popup loader #3157
Preview popup loader #3157
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3157 +/- ##
==========================================
- Coverage 31.66% 31.32% -0.35%
==========================================
Files 217 227 +10
Lines 6263 6414 +151
Branches 1112 1141 +29
==========================================
+ Hits 1983 2009 +26
- Misses 3600 3697 +97
- Partials 680 708 +28
Continue to review full report at Codecov.
|
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.
Flagged for design review in case one more design-oriented than myself has any feedback or suggestions 😄
<p>Generating preview.</p> | ||
</div> | ||
); | ||
const css = '<style type="text/css"> div { margin-top: 25%; } p { text-align: center; } </style>'; |
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.
Is the intention that this be centered? Seems like flexbox might serve better, if so.
@@ -73,6 +73,16 @@ export class PreviewButton extends Component { | |||
'about:blank', | |||
this.getWindowTarget() | |||
); | |||
|
|||
const popupLoader = renderToString( |
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.
Honestly seems like we could get away with just creating a string of HTML instead of using React to convert an element to string, particularly if we're separately creating a style tag anyways (which could itself be represented as an 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.
Good shout - I was only doing this because I'd originally used the <Spinner />
component. No longer necessary 🙂
<p>Generating preview.</p> | ||
</div> | ||
); | ||
const css = '<style type="text/css"> div { margin-top: 25%; } p { text-align: center; } </style>'; |
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.
type="text/css"
is not necessary since it is the default:
This attribute is optional and defaults to
text/css
if it's missing.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/style
); | ||
const css = '<style type="text/css"> div { margin-top: 25%; } p { text-align: center; } </style>'; | ||
this.previewWindow.document.write( css ); | ||
this.previewWindow.document.write( popupLoader ); |
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.
The docs for document.write
encourage calling document.close
after writing has completed:
Once you have finished writing, it is recommended to call
document.close()
to tell the browser to finish loading the page.
https://developer.mozilla.org/en-US/docs/Web/API/Document/write#Notes
); | ||
const css = '<style type="text/css"> div { margin-top: 25%; } p { text-align: center; } </style>'; | ||
</div>`; | ||
const css = ` |
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.
Should we just combine these two into a single variable of markup?
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.
That figures.
@aduth thanks for the comments - I've made those changes. Not sure whether it'd be better to keep the HTML/CSS strings to one line (not as readable), or leave them formatted with backticks as I've now done? |
This is fine as you've implemented. Code-wise, this looks good to me 👍 @jasmussen any thoughts on whether this deserves any additional design treatment? |
It'd be nice if we could use a sans-serif syste font instead of Times New Roman... something like the following font stack: |
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.
Looks good 👍 Will probably defer this one for merge after the upcoming release.
const markup = ` | ||
<div> | ||
<p>Please wait…</p> | ||
<p>Generating preview.</p> |
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.
We need to translate this.
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.
See #3301
Description
See #3087. Added a loading message for the preview popup.
Decided against adding a spinner/loading icon because we'd have to re-load the styles - thought this would be an over-kill for the sake of a second or two. Thoughts welcome on this & the message text...
How Has This Been Tested?
As per 533da25 - check that
document.write
is called when the preview popup is opened.Screenshots (jpeg or gifs if applicable):
Types of changes
Checklist: