Skip to content

feat: allow declaring document used manually using meta tags #95

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat: allow declaring document used manually using meta tags #95

wants to merge 7 commits into from

Conversation

lihbr
Copy link
Member

@lihbr lihbr commented Feb 10, 2022

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Overview

Following #94, this PR implements strategy 1 described there. Used documents can be explicitly declared using meta tags to help predictions.

Usage

Declaring the main document

<meta name="prismic-main-document" content="U0V72AEAAE4netWJ" />

None, or multiple instance of this meta tag will result in a warning being emitted.

Declaring other documents

<meta name="prismic-documents" content="U0V72AEAAE4netWJ,YfmNTRIAAC8AiccC" />

Multiple instance of this meta tags are allowed

Considerations & Limitations

Instead of bypassing the prediction process, I preferred to just make a query and let the prediction process track it. This allows for the prediction API to still resolve prediction-related data that aren't resolvable otherwise (document publication status, abstract, etc.)

This addition only works with public repositories for now.

Try it yourself

Just replace your toolbar script with this one and replace YOUR_REPO with yours:

https://prismic-toolbar-meta-predictions.netlify.app/prismic-toolbar/4.0.7/prismic.js?repo=YOUR_REPO&new=true

Should be working!

@lihbr lihbr requested a review from angeloashmore February 10, 2022 15:25
@lihbr
Copy link
Member Author

lihbr commented Feb 10, 2022

@angeloashmore, beyond the review, can you also try to check out if it's behaving as expected for you using the above toolbar script?

@angeloashmore
Copy link
Member

This is awesome @lihbr! Sorry it took me a while to review.

Everything looks great after testing. I only found one limitation that I think we should address (ignoring the private repo limitation for now, of course):

Reacting documents declared at runtime

In its current iteration, it seems like the toolbar will query for <meta> tags upon loading. When a user navigates to a different page, the toolbar reacts appropriately and re-runs its logic for the new page.

This misses any <meta> tags that may have been added at runtime. For example, a user might run a client-side request to fetch some documents and display them in a list. <meta> tags could be added for each document once the API returns responses, but the toolbar doesn't seem to check for <meta> tags after the initial page load (correct me if I'm wrong).

I encountered this while testing this with a Gatsby site in development mode. Gatsby client-side renders pages in development mode, meaning <meta> tags are not statically in the initial HTML. Instead, they are added at runtime, just like the CSR example.

(It's worth noting that this isn't a problem after building the site since the HTML will include those <meta> tags.)

I believe we could use a MutationObserver to accomplish this. By listening for changes to <head>, we can re-run the <meta> reading logic and update the list appropriately.

@lihbr
Copy link
Member Author

lihbr commented Feb 25, 2022

Here you go, updated the logic to also register a MutationObserver 🙂 Should be good now~

@lihbr
Copy link
Member Author

lihbr commented Mar 4, 2022 •
edited
Loading

Ready for test/review now @angeloashmore 🎉

@srenault following your review I updated the following:

  • Logic has been extracted into a file of its own for easier deprecation;
  • MutationObserver callback has been throttled to prevent spam;
  • I wasn't able to find a suitable way to consistently disable the historyChange event when meta tags are used, I keep thinking of some edge cases that will make it break one way or another. I think the throttling might be enough for now.

@angeloashmore
Copy link
Member

@lihbr Are you able to re-deploy the test to Netlify? Correct me if I'm wrong, but I don't think the updated Mutation Observer logic is deployed at https://prismic-toolbar-meta-predictions.netlify.app/prismic-toolbar/4.0.7/prismic.js?repo=YOUR_REPO&new=true

@lihbr
Copy link
Member Author

lihbr commented Mar 22, 2022

@lihbr Are you able to re-deploy the test to Netlify? Correct me if I'm wrong, but I don't think the updated Mutation Observer logic is deployed at prismic-toolbar-meta-predictions.netlify.app/prismic-toolbar/4.0.7/prismic.js?repo=YOUR_REPO&new=true

Here you go! My bad

@angeloashmore
Copy link
Member

angeloashmore commented Mar 22, 2022 •
edited
Loading

Thanks! 🙏 It's working great for me now.

I think it's good to merge and deploy (pending approval from others on the team).

Related:

I found an issue in the way react-helmet works (the primary library used to manage <head> in Gatsby projects): it replaces existing meta tags with the same name. This causes a problem with the model we are trying since a user won't be able to append new document IDs throughout a page's code.

In the following example, only the last document in the page.data.related_pages Group field would appear in prismic-documents:

{isFilled.group(page.data.related_pages) && (
  <ul>
    {page.data.related_pages.map((item) => {
      const document = item.related_page.document;

      return (
        document && (
          <li key={document.prismicId}>
            <Helmet>
              <meta
                name="prismic-documents"
                content={document.prismicId}
              />
            </Helmet>
            <PrismicLink href={document.url}>
              {document.prismicId} &mdash; {document.data.title.text}
            </PrismicLink>
          </li>
        )
      );
    })}
  </ul>
)}

We may need to provide an abstraction for React and/or Gatsby users. Even with this limitation, I believe <meta> elements are better than appending data attributes to arbitrary elements.

Note that Next.js does not perform this overriding automatically (see the key prop details), so this issue seems specific to users using react-helmet.

@lihbr lihbr requested a review from srenault March 23, 2022 11:50
@lihbr
Copy link
Member Author

lihbr commented Mar 23, 2022

Alright, thanks!

@srenault ready for review/test on staging

@angeloashmore angeloashmore mentioned this pull request Jun 15, 2023
Closed
7 tasks
@leopoldkristjansson
Copy link

I would really like to have this feature. I think the toolbar is a smart simple solution, but is simply not reliable enough it seems. I often get the wrong or missing documents. Often it simply does not show up at all.

Also see: https://community.prismic.io/t/prismic-toolbar-not-showing-correct-documents-sometimes-not-showing-at-all/15114/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srenault srenault srenault approved these changes

@angeloashmore angeloashmore Awaiting requested review from angeloashmore

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lihbr @angeloashmore @leopoldkristjansson @srenault