Improve demo store SEO & ld+json implementation #598

juanpprieto · 2023-02-28T19:27:05Z

WHY are these changes introduced?

Fixes first task of #593

TODO:

Add collection seo + ldJson
Add journal/blog seo + ldJson
Add pages seo + ldJson

WHAT is this pull request doing?

Done

Moved root.tsx seo to loader pattern and added Organization ldJson
added ldJson to products/$productHandle based on Dawn Theme.
added a couple of wip analyticsPayload and seoPayload helpers to both templates to make them improve the DX and cosistency.

Issues:

ldJson currently does not allow for multiple tags to be defined at root and route level. This is a pattern in Dawn so we might need to change the ldJson API to allow for this. We are discussing with Paul Shapiro.

Suggestion:

Instead of having a hook useAnalytics should we instead have a <Analytics /> component to better align with the Seo component and remix's own Script, Head etc...

Change this

export default function App() {
  const data = useLoaderData<typeof loader>();
  const locale = data.selectedLocale ?? DEFAULT_LOCALE;
  const hasUserConsent = true;

  useAnalytics(hasUserConsent, locale);

  return (
    <html lang={locale.language}>
      <head>
        <Seo />
        <Meta />
        <Links />
      </head>
      <body>
        <Layout
          layout={data.layout as LayoutData}
          key={`${locale.language}-${locale.country}`}
        >
          <Outlet />
        </Layout>
        <ScrollRestoration />
        <Scripts />
      </body>
    </html>
  );
}

to

export default function App() {
  const data = useLoaderData<typeof loader>();
  const locale = data.selectedLocale ?? DEFAULT_LOCALE;
  const hasUserConsent = true;

  return (
    <html lang={locale.language}>
      <head>
        <Seo />
        <Meta />
        <Links />
      </head>
      <body>
        <Layout
          layout={data.layout as LayoutData}
          key={`${locale.language}-${locale.country}`}
        >
          <Outlet />
        </Layout>
        <ScrollRestoration />
        <Analytics userConsent={hasUserConsent} locale={locale} />
        <Scripts />
      </body>
    </html>
  );
}

HOW to test your changes?

Post-merge steps

Checklist

I've read the Contributing Guidelines
I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've added a changeset if this PR contains user-facing or noteworthy changes
I've added tests to cover my changes

wizardlyhel · 2023-02-28T20:08:54Z

I'm good with the analyticPayload clean up but to convert useAnalytics hook to a component only for consistency is weird. useAnalytics hook doesn't need to render anything.

blittle · 2023-02-28T20:21:37Z

I'm good with the analyticPayload clean up but to convert useAnalytics hook to a component only for consistency is weird. useAnalytics hook doesn't need to render anything.

A component doesn't need to render anything. There's a lot of precedence in the React ecosystem to have components that don't render. I think the bigger question is the dev ergonomics. Do we ever expect useAnalytics to return some sort of data? If it's a component, then you gotta use render props to pass data out. Pros and cons to both.

juanpprieto · 2023-02-28T20:59:52Z

I agree. From a DX perspective it feels much more love-letter-to-remix to have both <Analytics /> and <Seo /> as components.

<Analytics /> would just be a simple wrapper for useAnalytics after all similar to how useFetcher.form wraps Form under the hood.

frehner · 2023-02-28T21:08:27Z

Slight preference on the "custom hook" form over a "component that wraps a custom hook" form, but it's not strongly held. I don't see the need to make a component that does nothing, when custom hooks are a React ecosystem thing as well (and are also widely used).

blittle · 2023-02-28T21:32:30Z

I think my preference also leans towards the hook, because I totally imagine that the hook could return something, and I think it's easier to adapt.

benjaminsehl

Edit: Approved by accident, meant to comment — since this is WIP and didn't top hat, can't approve.

If deleting the component is a shortcut to removing analytics reporting full stop -- that is kind of a nice escape hatch in an exploration of HIPAA compliance.

I don't feel really strongly either way; but I do think consistency of ergonomic is a good principle to follow -- so long as it doesn't compromise the overall DX or performance.

Likewise -- consistency here might mean we move to create a "useSeo" hook in hydrogen-react instead of a component for Analytics -- it might also mean that we say "we will consistently use components when we return markup, hooks when we don't".

I still have to dig through the SEO proposals but I'd say we should separate the two ideas -- don't necessarily shoot down the Analytics component idea, but don't commit to merging yet either. Just let it simmer for a bit and get some dogfooding in to see how it feels.

On the SEO stuff…

This DX is so much cleaner and easier to follow -- love this direction.

Also, glad you're using Brand Settings API. We should push to see if they can expand what's exposed to include all of the social links, etc. so we can make OG defaults use Brand as the source of truth. Makes it a really nice clean place to edit a single screen and have your full app get up to date.

cartogram

🚀 @juanpprieto Thank you for helping with this!

I actually like the component that renders nothing pattern.... makes it easy to be consistent (you can move a hook into a component, but its awkward/sometimes impossible to go the the other way). So @benjaminsehl we can't do the useSeo hook because we produce markup (we could explore this limitation more though).

I think there has always been a good amount of sharing between seo and analytics in trying to come up with consistent APIs. Its great to see that continue to play out here ❤️

cartogram · 2023-02-28T21:45:17Z

templates/demo-store/app/root.tsx

+      primaryDomain {
+        url
+      }
+      brand {


Future thing: If we move the helpers into the hydrogen-react package (they not are tied to remix/framework agnostic) we should warn and provide lint rules for missing request data. It's easy to miss and prone to confusion, but I don't think its a dealbreaker here since we rely on this pattern all the time. If anything, this will further encourage us to provide some DX to bridge the "I am using this thing" and "I need to add xyz fields to the query".

cartogram · 2023-02-28T21:55:42Z

templates/demo-store/app/routes/($lang)/products/$productHandle.tsx

+  };
+}
+
+function seoPayload({


Nit: I think add the word product to these ones. ie: seoProductPayload or getProductSeo, but not interested in bikeshedding names at this point.

I think it's worth bikeshedding! Less about this specific one and more for the precedent of the pattern overall.

From a design elegance POV -- I like that it's a single function; maybe could accept an arg for the resource type.

However -- I'm being naive about in that suggestion? Is there a reason to be more imperative w/ function names? Like would it be a lot more prone to getting things wrong or something?

Cool, I was thinking of maybe moving these utils to a seo/ and analytics/ folders inside the demo store. I'm easy with bikeshedding their names, but not sure these should ever move to hydrogen as they are very project-specific.

frehner · 2023-02-28T22:23:13Z

So @benjaminsehl we can't do the useSeo hook because we produce markup (we could explore this limitation more though).

Can you clarify this? Are you producing JSX or are you producing something else? (I was under the impression that you were producing something else and then turning it into JSX, but I could be wrong. Maybe that's only the hydrogen-react one that I'm thinking about?)

cartogram · 2023-02-28T22:27:19Z

Can you clarify this? Are you producing JSX or are you producing something else? (I was under the impression that you were producing something else and then turning it into JSX, but I could be wrong. Maybe that's only the hydrogen-react one that I'm thinking about?)

Exactly. The component still returns JSX (its the react/remix bridge). The part that returns the abstract dom objects are the hydrogen-react part.

Co-authored-by: Matt Seccafien <[email protected]>

migrate analytic payloads helpers to lib/analytics seo + analytics demo store implementation cleanup analytics refactor

juanpprieto · 2023-03-07T05:16:00Z

Moved to #651

juanpprieto and others added 4 commits February 28, 2023 07:51

add products route ld+json and refactor analytics and seo helpers

77d34aa

change root.tsx analytics to loader pattern and add ldJson

bfa8d5c

cleanup unused imports

5b9fc85

cleanup unused variables

492e33b

benjaminsehl approved these changes Feb 28, 2023 •

edited

Loading

View reviewed changes

benjaminsehl self-requested a review February 28, 2023 21:51

benjaminsehl reviewed Feb 28, 2023

View reviewed changes

cartogram approved these changes Feb 28, 2023

View reviewed changes

frandiox and others added 3 commits February 28, 2023 16:27

Refresh app when hydrogen-react changes (#595)

32876e9

Hard code date in robots tag (#597)

1573412

Co-authored-by: Matt Seccafien <[email protected]>

improve root and productHandle types

607aca7

migrate analytic payloads helpers to lib/analytics seo + analytics demo store implementation cleanup analytics refactor

github-actions bot deployed to preview March 6, 2023 23:55 View deployment

juanpprieto changed the title ~~WIP: Extend demo store SEO implementation~~ Improve demo store SEO & ld+json implementation Mar 7, 2023

juanpprieto closed this Mar 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve demo store SEO & ld+json implementation #598

Improve demo store SEO & ld+json implementation #598

juanpprieto commented Feb 28, 2023 •

edited

Loading

wizardlyhel commented Feb 28, 2023

blittle commented Feb 28, 2023

juanpprieto commented Feb 28, 2023 •

edited

Loading

frehner commented Feb 28, 2023

blittle commented Feb 28, 2023

benjaminsehl left a comment •

edited

Loading

cartogram left a comment

cartogram Feb 28, 2023 •

edited

Loading

cartogram Feb 28, 2023

benjaminsehl Feb 28, 2023

juanpprieto Feb 28, 2023 •

edited

Loading

frehner commented Feb 28, 2023

cartogram commented Feb 28, 2023 •

edited

Loading

juanpprieto commented Mar 7, 2023

Improve demo store SEO & ld+json implementation #598

Improve demo store SEO & ld+json implementation #598

Conversation

juanpprieto commented Feb 28, 2023 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

HOW to test your changes?

Post-merge steps

Checklist

wizardlyhel commented Feb 28, 2023

blittle commented Feb 28, 2023

juanpprieto commented Feb 28, 2023 • edited Loading

frehner commented Feb 28, 2023

blittle commented Feb 28, 2023

benjaminsehl left a comment • edited Loading

Choose a reason for hiding this comment

cartogram left a comment

Choose a reason for hiding this comment

cartogram Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

cartogram Feb 28, 2023

Choose a reason for hiding this comment

benjaminsehl Feb 28, 2023

Choose a reason for hiding this comment

juanpprieto Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

frehner commented Feb 28, 2023

cartogram commented Feb 28, 2023 • edited Loading

juanpprieto commented Mar 7, 2023

juanpprieto commented Feb 28, 2023 •

edited

Loading

juanpprieto commented Feb 28, 2023 •

edited

Loading

benjaminsehl left a comment •

edited

Loading

cartogram Feb 28, 2023 •

edited

Loading

juanpprieto Feb 28, 2023 •

edited

Loading

cartogram commented Feb 28, 2023 •

edited

Loading