magento-research / peregrine

(Defunct) Moved to https://github.com/magento-research/pwa-studio
Open Software License 3.0
29 stars 1 forks source link

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

Open zetlen opened 6 years ago

zetlen commented 6 years ago

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

magento-cicd2 commented 6 years ago

CLA assistant check
All committers have signed the CLA.

PWAStudioBot commented 6 years ago

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 :no_entry_sign: dangerJS

DrewML commented 6 years ago

Leaving @jimbo's Slack comment here:

image

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

DrewML commented 6 years ago

(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 commented 6 years ago
  1. 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?

  2. Thanks for the test suggestion; results will definitely be more informative now.

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

DrewML commented 6 years ago

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.

brendanfalkowski commented 6 years ago

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 commented 6 years ago

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

zetlen commented 6 years ago

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

DrewML commented 6 years ago

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