google / react-schemaorg

Type-checked Schema.org JSON-LD for React
Apache License 2.0
487 stars 19 forks source link

Extend JsonLd component to accept stringify's space property #14

Closed ntucakovic closed 4 years ago

ntucakovic commented 4 years ago

I have quite long JSON-LD schemas, and it would be very useful if we could support passing down space property down to JSON stringify.

Something like:

export class JsonLd<T extends Thing> extends React.Component<{
  item: WithContext<T>;
  space?: string | number;
}> {
  render() {
    return (
      <script
        type="application/ld+json"
        dangerouslySetInnerHTML={{
          __html: JSON.stringify(this.props.item, safeJsonLdReplacer, this.props.space)
        }}
      />
    );
  }
}

I could do a PR with this

Eyas commented 4 years ago

Can you elaborate on why the default isn't appropriate? Typically we expect the output of this to be only used to be read by machines (indexers, etc) and so having it as short as possible makes sense (the default when space is not set).

ntucakovic commented 4 years ago

It's totally appropriate to have default as it currently is, I wouldn't change that. I'm only suggesting to allow the React component to pass down space property to stringify method.

I'm porting over existing JSON-LD declared straight in HTML to React, and having both at the same time nicely printed allows me to compare the difference easily, without having to rely on some other.

Eyas commented 4 years ago

Got it. Yeah cleaner diffs sounds like a compelling reason here, I get the pain.

Writing a PR with this change sounds good!