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

Custom HTML Block: apply editor-styles to preview mode. #13080

Merged
merged 10 commits into from
Jan 18, 2019
Merged

Custom HTML Block: apply editor-styles to preview mode. #13080

merged 10 commits into from
Jan 18, 2019

Conversation

torounit
Copy link
Member

Description

Apply Editor Styles for custom html block preview.

  • add export urlRewrite, traverse, wrap in @wordpress/editor.
  • add css prop to <Sandbox>

Screenshots

before

2018-12-22 23 28 29

after

2018-12-22 23 27 24

theme: Twenty Nineteen.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

return {
styles: [],
};
} ),
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is not the fastest one and the main issue is that it will be executed on each subscribe (on each state change). Should we mirror how it's done in the EditorProvider component (when mounting the component)?

Also there's some duplication between these two components. Do you think we can extract the logic somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

@youknowriad

Does it mean you need to cache styles?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean just compute the styles prop on initial mount of the component, store it as state or something and avoid computing it on each render.

@torounit
Copy link
Member Author

torounit commented Jan 9, 2019

@youknowriad I think fixed problem. Cached computed styles.

@youknowriad
Copy link
Contributor

Thanks for the update @torounit This works. I'd prefer if we avoid the global variable though and tie it to the component.

Something like this

https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/provider/index.js#L49-L68

In fact the logic is the same and should probably be extracted to a utility.

@torounit
Copy link
Member Author

torounit commented Jan 9, 2019

@youknowriad
Transform css logic in packages/editor/src/editor-styles. How is this change?

@youknowriad
Copy link
Contributor

Changes are great. I'd still prefer to avoid the let editorStylesCache = []; and computing the CSS in withSelect and instead use componentDidMount for that. (like the provider).

I'll add more reviewers here. I'm mostly happy with this change, I think we're very close.

@youknowriad youknowriad requested a review from a team January 10, 2019 07:52
@torounit
Copy link
Member Author

@youknowriad

how about latest update ?

@youknowriad
Copy link
Contributor

Hi @torounit I pushed a small update, I hope you don't mind. Let me know what you think.

@jasmussen how this looks for you?

@youknowriad youknowriad added this to the 4.9 (Gutenberg) milestone Jan 11, 2019
@youknowriad youknowriad added [Feature] Custom Editor Styles Functionality for adding custom editor styles [Type] Enhancement A suggestion for improvement. labels Jan 11, 2019
@jasmussen
Copy link
Contributor

Judging purely by the screenshot, this seems pretty good to me, 👍 👍

@youknowriad youknowriad merged commit 9fc611e into WordPress:master Jan 18, 2019
@torounit torounit deleted the feature/custom-html-preview-styles branch January 18, 2019 13:48
@torounit
Copy link
Member Author

👍

@talldan
Copy link
Contributor

talldan commented Jan 22, 2019

Hey @torounit - I spotted an issue that seems to have resulted from this PR, see #13414.

Just wondering if you have any thoughts on what a potential fix might look like?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Custom Editor Styles Functionality for adding custom editor styles [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants