feat(components): RichContent component for server HTML #47

zetlen · 2018-06-07T20:54:46Z

This PR is a:

[x] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

PageBuilder CMS, like many CMSes, will provide raw HTML as a server
response. This PR adds a RichContent component to handle that common use
case.

When this pull request is merged, it will...

Add the RichContent component, its stories, docs, and tests, and expose it as an exported property at the root.

Additional information

It uses the dangerouslySetInnerHTML API, rendering a string of
HTML supplied as a prop.
If the outer system replaces the raw HTML with progressively
enhanced functionality, such as a tree of live ReactElements,
RichContent accepts ReactElements as children. Children will
override the sanitizedRawHtml prop.

magento-cicd2 · 2018-06-07T20:54:48Z

All committers have signed the CLA.

PWAStudioBot · 2018-06-07T20:57:37Z

All tests passed! View Coverage Report

A Storybook for this PR has been deployed!. It will be accessible as soon as the current build completes.

Generated by 🚫 dangerJS

DrewML · 2018-06-08T14:33:21Z

src/Content/RichContent.js

+    render() {
+        return (
+            this.props.children || (
+                <span


This should probably be a fragment, since the contents below the span could be block level elements

Good eye, I'll make that change.

Well, turns out you can't dangerouslySetInnerHTML on a Fragment, so it'll have to be a wrapper element. But I'll let 'em specify what it is, and allow props spread.

DrewML · 2018-06-11T15:10:51Z

Leaving @jimbo's Slack comment here:

I don't know that the name RichContent satisfies this.

DrewML · 2018-06-11T15:22:14Z

src/Content/RichContent.js

+        return (
+            this.props.children || (
+                <span
+                    className="peregrine-raw-html"


We're not encouraging use of global CSS, so I don't think it's a good idea to have a generic class on here. Would prefer className={this.props.className}

DrewML · 2018-06-11T15:24:46Z

src/Content/__tests__/RichContent.test.js

+    expect(wrapper.html()).toEqual(
+        '<span class="peregrine-raw-html"><h1 id="o-no">Raw!!!</h1></span>'
+    );
+    wrapper = mount(


This should be a separate test case. It's important that, if this one assertion fails, the description of the test failing is clear. Right now, if we have a bug that favors the null children, the name of the test failing would be Renders raw HTML, which isn't descriptive of what we're testing (specifically, that we ignore null children and continue to favor the sanitizedRawHtml prop).

Good point; I've changed it.

DrewML · 2018-06-11T15:26:46Z

(As you probably already know), I am a big fan of keeping API surfaces as small as we possibly can until we have an actual need for an API. In this case, I'm not convinced we need the children enhancement behavior.

How do you feel about sticking to just the prop until we have a real-world reason to accept children?

zetlen · 2018-06-12T13:18:51Z

I chose the RichContent name because:
- React isn't designed to display HTML from elsewhere
- Magento's CMS functionality creates and stores structured content (serialized HTML from the WYSIWYG and/or PageBuilder) on the server sid
- Including such content will be a pretty common use case, so the component name should be short and smooth
- The component is for displaying content with rich capabilities, even if that just means "formatting"
I considered RawHTML and ContentWithFallback, but I thought both of them would get a few more double takes from devs than RichContent would.

I'm happy to change it for a better suggestion, though. @DrewML @jimbo @jcalcaben do you have one?
Thanks for the test suggestion; results will definitely be more informative now.
The API I wrote is what I think is minimally viable for the PageBuilder integration strategy we currently have. In that proposal, raw HTML comes down first and is then replaced with live implementations once their components and data load.

Still, I could remove the children override from the API like you're suggesting, because it would still have business value for legacy WYSIWYGs. I'll make that change and push it (and save the children API for later, if our current PB integration path stays the same).

PageBuilder CMS, like many CMSes, will provide raw HTML as a server response. Added a `RichContent` component to handle this common use case. - It uses the `dangerouslySetInnerHTML` API, rendering a string of HTML supplied as a prop. - If the outer system replaces the raw HTML with progressively enhanced functionality, such as a tree of live `ReactElements`, `RichContent` accepts `ReactElements` as children. Children will override the `sanitizedRawHtml` prop. feat(RichContent): Add configurability to wrapper tag - Added two props to configure the "parent element" that React requires to `dangerouslySetInnerHTML`: - `wrapperTag` to specify the tag name as a string - `wrapperProps` to pass an object of props to that tag - Updated docs, stories, and tests to reflect this change feat:retract children API for later

DrewML · 2018-06-12T15:17:39Z

Still, I could remove the children override from the API like you're suggesting, because it would still have business value for legacy WYSIWYGs

That would be great. And not just "legacy WYSIWYG" - this is useful today for Category Description on category pages, which can (and frequently do) contain raw markup.

I'm all for the fallback thing when we get there. I just don't want to take a stab at the API now, because if we're wrong, we're stuck supporting it + something new.

DrewML · 2018-06-12T15:19:50Z

src/Content/RichContent.js

+    };
+
+    render() {
+        // JSX tags must be in PascalCase


FWIW, this is only accurate for bare identifiers. Member expressions do not have this limitation.

DrewML · 2018-06-12T15:23:52Z

src/Content/RichContent.js

+
+    static defaultProps = {
+        wrapperTag: 'span',
+        wrapperProps: {


From what I've seen, it's more common in community React components to just spread any non-own prop into the wrapper in this case. So your render would be:

render() { const { wrapperTag: Tag, ...others } = this.props; return ( <Tag className='peregrine-raw-html' {...others} /> ); }

and you drop wrapperProps entirely

I'm so accustomed to not using object spread for support reasons that I forgot it was an option. I'll change the API.

DrewML · 2018-06-12T15:24:34Z

src/Content/index.js

@@ -0,0 +1 @@
+export { default as RichContent } from './RichContent';


This should also be re-exported in src/index.js

brendanfalkowski · 2018-06-12T18:13:30Z

I considered RawHTML and ContentWithFallback, but I thought both of them would get a few more double takes from devs than RichContent would.

RichContent feels like marketing-speak and I'd personally double-take what that is exactly. MS WordPad, anyone? RawHTML or PlainHTML or just HTML seem to describe what's in the tin better.

DrewML · 2018-06-12T18:18:05Z

I'll throw my vote in for RawHTML - Plain doesn't seem super clear to me.

zetlen · 2018-06-13T09:12:35Z

@brendanfalkowski @DrewML Thanks for the feedback, folks. I may be trying to solve a problem of a repetitive pattern I'm anticipating, one that would look a little like this:

 <Query query={CMS_CONTENT} variables={{ id }}>
    {({ loading, error, data }) => {
      if (loading) return null;
      if (error) return `Error!: ${error}`;
      if (data.children) {
          return data.children;
      }
      return (<RawHtml html={data.raw} />)
    }}
  </Query>

But that may end up not being a big hassle. I'll change the component name to RawHtml.

- Replace `wrapperProps` with an object spread that automatically removes the custom props from the remaining object - Export component from root

DrewML · 2018-06-13T23:03:29Z

This PR will need to be re-targeted to the new monorepo.

DrewML reviewed Jun 8, 2018

View reviewed changes

DrewML reviewed Jun 11, 2018

View reviewed changes

zetlen force-pushed the zetlen/rich-content-component branch from d19436d to 20d2b3b Compare June 12, 2018 13:27

DrewML reviewed Jun 12, 2018

View reviewed changes

style: rename component to RawHtml

5e26edf

zetlen force-pushed the zetlen/rich-content-component branch from 280d8d2 to f6f27a6 Compare June 13, 2018 09:35

style: switch DOM props to spread

f0b4217

- Replace `wrapperProps` with an object spread that automatically removes the custom props from the remaining object - Export component from root

zetlen force-pushed the zetlen/rich-content-component branch from a0e78a3 to f0b4217 Compare June 13, 2018 09:53

Merge branch 'master' into zetlen/rich-content-component

9cc4f4f

		@@ -0,0 +1 @@
		export { default as RichContent } from './RichContent';

feat(components): RichContent component for server HTML #47

Are you sure you want to change the base?

feat(components): RichContent component for server HTML #47

Uh oh!

Conversation

zetlen commented Jun 7, 2018

This PR is a:

Summary

Additional information

Uh oh!

magento-cicd2 commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PWAStudioBot commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrewML commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrewML Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrewML commented Jun 11, 2018

Uh oh!

zetlen commented Jun 12, 2018

Uh oh!

DrewML commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrewML Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendanfalkowski commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrewML commented Jun 12, 2018

Uh oh!

zetlen commented Jun 13, 2018

Uh oh!

DrewML commented Jun 13, 2018

Uh oh!

Uh oh!

magento-cicd2 commented Jun 7, 2018 •

edited

Loading

PWAStudioBot commented Jun 7, 2018 •

edited

Loading

DrewML Jun 11, 2018 •

edited

Loading

DrewML Jun 12, 2018 •

edited

Loading

brendanfalkowski commented Jun 12, 2018 •

edited

Loading