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

Preview popup loader #3157

Merged
merged 9 commits into from
Nov 1, 2017
Merged

Preview popup loader #3157

merged 9 commits into from
Nov 1, 2017

Conversation

jack-lewin
Copy link
Contributor

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):

gutenberg-popup-preview

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@codecov
Copy link

codecov bot commented Oct 25, 2017

Codecov Report

Merging #3157 into master will decrease coverage by 0.34%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
editor/header/preview-button/index.js 95.83% <100%> (+0.59%) ⬆️
editor/sidebar/post-author/index.js 0% <0%> (-85%) ⬇️
editor/sidebar/post-pending-status/index.js 0% <0%> (-58.34%) ⬇️
editor/sidebar/post-sticky/index.js 0% <0%> (-54.55%) ⬇️
components/navigable-menu/index.js 25.92% <0%> (-2.08%) ⬇️
editor/utils/dom.js 15.07% <0%> (-1.03%) ⬇️
editor/selectors.js 95.21% <0%> (-0.44%) ⬇️
editor/block-toolbar/index.js 0% <0%> (ø) ⬆️
editor/layout/index.js 0% <0%> (ø) ⬆️
editor/sidebar/post-format/index.js 0% <0%> (ø) ⬆️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec8df21...739880a. Read the comment docs.

@aduth aduth added the Needs Design Feedback Needs general design feedback. label Oct 25, 2017
Copy link
Member

@aduth aduth left a 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>';
Copy link
Member

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(
Copy link
Member

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).

Copy link
Contributor Author

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>';
Copy link
Member

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 );
Copy link
Member

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 = `
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That figures.

@jack-lewin
Copy link
Contributor Author

@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?

@aduth
Copy link
Member

aduth commented Oct 28, 2017

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?

@jasmussen
Copy link
Contributor

@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: -apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,Oxygen-Sans,Ubuntu,Cantarell,"Helvetica Neue",sans-serif, but other than that, 👍 👍!

Copy link
Member

@aduth aduth left a 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.

@aduth aduth merged commit 22af16c into WordPress:master Nov 1, 2017
const markup = `
<div>
<p>Please wait&hellip;</p>
<p>Generating preview.</p>
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

See #3301

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants