-
Notifications
You must be signed in to change notification settings - Fork 19
Conversation
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.
This looks reasonable to me. 👍
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.
I haven't attempted to use this in stumptown-renderer
but if I think about it, I think it'll work nicely and be easy to do.
And my prototype I have on the renderer for syntax highlighting would work when the packaged content is like this.
Almost all of my comments are nits but I really think we should avoid console.error
and throw a proper error instead because swallowing problems are dangerous.
// TODO: implement packaging for info boxes | ||
return 'info_box-value'; | ||
default: | ||
console.error(`Error: Unrecognized ingredient: ${ingredient}`); |
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.
console.error(`Error: Unrecognized ingredient: ${ingredient}`); | |
throw new Error(`Unrecognized ingredient: ${ingredient}`); |
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.
If you like this suggestion, the next line would need to be nixed.
const matches = proseSections.filter(section => section.value.id === ingredientName); | ||
if (matches.length > 0) { | ||
return matches[0]; | ||
} |
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.
In the else
case, should it at least return null
or perhaps throw an error? Either way, looks a bit "naked" without dealing with this.
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.
I think it is permissible for there not to be any matches, if the prose ingredient is optional and this item omits it. But I agree that we should have an explicit return branch here.
} else if (ingredientType === 'prose') { | ||
return await processProseIngredient(ingredientName, proseSections); | ||
} else { | ||
throw (`Error: Unrecognized ingredient type: ${ingredientType}`); |
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.
Shouldn't this be...
throw new Error(`Unrecognized ingredient type: ${ingredientType}`);
Also, perhaps it's overkill but I could see how this error might be very cryptic if it happens in a CI build. We could either do something like this:
console.warn(`Ingredient processing problem in ${elementPath}`);
throw new Error(`Unrecognized ingredient type: ${ingredientType}`);
...or...
throw new Error(`Unrecognized ingredient type: ${ingredientType} in ${elementPath}`);
Co-Authored-By: Peter Bengtsson <peterbe@mozilla.com>
Co-Authored-By: Peter Bengtsson <peterbe@mozilla.com>
Co-Authored-By: Peter Bengtsson <peterbe@mozilla.com>
Co-Authored-By: Peter Bengtsson <peterbe@mozilla.com>
Thanks for the reviews! @peterbe , I think all your comments should be addressed? @Elchi3 , since this PR affects the external interface I'd love it if you could take a look but I appreciate you are busy. So let me know if you expect to get a chance to look at this, and if you would like me to wait until you can find time. |
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.
Seems my few things are fully addressed. Yay!
Perhaps I'm missing something. The packaged file
What we need to achieve is a DOM so that links like this work. Is that supposed to be done in the renderer now? |
In
So, a React component would know what to do. E.g. return <div>
<h2 id={section.id}>{section.title}</h2>
<div dangerouslyInsertHTML={{__html: section.content}}/>
</div> |
The
There's a basic difference between the reference doc JSON and the guides JSON. In the reference docs, For example, a guide doc could have:
....this would get processed into 5 pieces:
The third piece is a prose section starting with a But then, even for reference docs, the I feel like if it's an important use case to have |
My gut tells me this should be done in stumptown-content. Perhaps we'll get 99% mileage from doing it with the |
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.
@Elchi3 , since this PR affects the external interface I'd love it if you could take a look but I appreciate you are busy. So let me know if you expect to get a chance to look at this, and if you would like me to wait until you can find time.
Sorry for not being involved. I'm bad at context switching and BCD has been my focus this sprint.
I've read the discussion in the issue and in this PR and from a conceptual point of view, this makes sense to me and I'm looking forward to seeing this new public interface in action. I haven't reviewed details or the code. It seems others have done that already.
(Part of me wishes to start a side project that uses stumptown-content, so that I could give you way better feedback from a consumers point of view that is not stumptown-renderer aka "mdn the website")
mdn/yari#85 hint hint :) |
Wohoo!! I'll try to take a stab at using this in the renderer tomorrow. |
This PR is an attempt to implement something like #68 (comment).
It changes the build-json stuff to emit something like:
That is, is has
title
,mdn-url
, thenrelated_content
. Thenbody
which is an array of the bits of the page.Each element in the
body
array is an object where:type
is the type of thing, eitherprose
for a simple prose section or an identifier likeinteractive_example
value
is the value of the thing and its syntax is dependent on the value oftype
.The elements in
body
are listed in the order we expect them to be in the page (which is also the order they're listed in the recipe.Features of this are that build-json is pretty generic: it has to add code when we invent a new atomic piece of a page, but not just for a new recipe (so for example this should be able to handle the proposed input element type without any changes). And then the renderer is generic: again it has to know how to render each atomic piece, but to render any page it just has to walk through the array rendering each piece in order.
Another feature of course is that the JSON output is much closer to the MDN page than the current one in, and we'd have to decide, eventually, whether that's a direction we want to go in.
This PR also comes with the change proposed for
info_box
(#106). It would be possible to keep the original syntax, although it would make the processing more complicated. But I don't think keeping the old syntax would be the right thing to do. I'm still not sure what we should do with "info boxes" - most reference pages have them in one form or another and it would be good to invent a generic syntax for them.