Open lihbr opened 2 years ago
@angeloashmore, beyond the review, can you also try to check out if it's behaving as expected for you using the above toolbar script?
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):
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.
Here you go, updated the logic to also register a MutationObserver 🙂 Should be good now~
Ready for test/review now @angeloashmore :tada:
@srenault following your review I updated the following:
MutationObserver
callback has been throttled to prevent spam;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.@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 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
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} — {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
.
Alright, thanks!
@srenault ready for review/test on staging
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.
Types of changes
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
Declaring other documents
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:Should be working!