-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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.
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
[1.0] For Contentful: filter out unresolvable entries and create markdown text nodes #1202
Conversation
@mericsson would love a review if you have a bit of time. |
Deploy preview ready! Built with commit 16e879b |
Deploy preview ready! Built with commit 16e879b |
Deploy preview ready! Built with commit 16e879b |
(and btw, spot on work with the Contentful plugin! Worked perfectly once I figured out the |
// links. | ||
const notResolvable = new Map() | ||
entryList.forEach(ents => { | ||
if (ents.errors) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blanking right now.. why would there be non-resolvable ids?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place I found it documented is here: http://cocoadocs.org/docsets/ContentfulDeliveryAPI/1.9.2/Classes/CDAArray.html
Basically says if a referenced entry is deleted but the reference also isn't also deleted they send this error 🤷♂️
The client I'm working with has a ton of content types and has been doing a lot of experimenting so this came up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, cool. Good to use it with real existing sites. :)
mediaType: `application/json`, | ||
}, | ||
} | ||
|
||
// Replace text fields with text nodes so we can process their markdown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, in a separate project I ended up doing...
<div dangerouslySetInnerHTML={{ __html: md.render(content) }} />
... with a text field using MarkdownIt
. Having built in support will be nice. I can update the using-contentful
site later to exercise this functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, doing markdown conversion at compile time is much nicer. Much less client code + somewhat faster render times.
// Create a node for each content type | ||
const contentTypeItemStr = JSON.stringify(contentTypeItem) | ||
const contentTypeItemStr = stringify(contentTypeItem) | ||
|
||
const contentTypeNode = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really in scope for this review, but I wonder if it was a good idea to expose ContentType itself as a gatsby node. I haven't thought of a use case for it yet. I suppose there's no particular harm, but could consider removing it to lower the plugin complexity / surface area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wondered about this too...
One use case perhaps is someone building an internal documentation site from their content model.
I'd lean towards keeping it I guess. I assume Contentful exposes it because there's sufficient demand?
@Khaledgarbaya could you provide background for what people generally use this data for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @KyleAMathews I've seen user generally get ContentTypes if they want to map content types to components, for example, to have some sort of dynamic fields etc. Bu the normal usage I see user mostly care about Entries and assets
Looks good to me! As you said before, it will be nice to get the sync in at some point. Feels lame using API requests during development when restarting the server. Maybe I'll look at that some point... |
Sweet! Thanks for the review! I'll merge this then and make a quick canary release. |
Also, looked a bit more at their ImageAPI and it'd be very easy to port over most of the functionality I added to gatsby-transformer-sharp for image queries. E.g. do stuff like: contentfulArticle {
title
text {
childMarkdownRemark {
html
}
}
image {
responsiveSizes(maxWidth: 800) {
src
srcSet
sizes
}
}
} This would generate responsive image urls with their image api that you could just drop in an image tag. If they added support for generating a base64 string then we could do the blur-up effect really easily too. |
No description provided.