Skip to content
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

Render example sections #158

Merged
merged 1 commit into from
Oct 2, 2019
Merged

Conversation

joedarc
Copy link
Contributor

@joedarc joedarc commented Oct 1, 2019

Fixes #137

Please don't merge until #156 is merged into master.

This will allow example sections to render properly. Fixes #137 and partially satisfies #157

@@ -92,3 +92,7 @@ export function Examples({ examples }) {
</>
);
}

export function Example({ example }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just put export in front on line 65.
Also, to align with the style of naming, would you mind renaming RenderExample to Example. and do the necessary renaming on line 90.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearly the result of a late night PR..apologies 😅

@peterbe peterbe requested a review from wbamberg October 1, 2019 12:03
@peterbe
Copy link
Contributor

peterbe commented Oct 1, 2019

Apart from the nit I've mentioned I think @joedarc has completely implemented based on this comment: #137 (comment)

Perhaps we're being naive and perhaps you have bigger plans for dealing with example and examples in a joint way. If I remember correctly, we'd need to figure out where to put the English string "Examples" (to be put into a <h2> tag.)

@joedarc joedarc force-pushed the render-example-sections branch from fe0dbb5 to a227224 Compare October 1, 2019 13:55
@wbamberg
Copy link
Collaborator

wbamberg commented Oct 1, 2019

If I remember correctly, we'd need to figure out where to put the English string "Examples" (to be put into a <h2> tag.)

Yes, it seems convincing to suggest that these strings should be maintained in stumptown-content. As for a joint way to handle "examples" and "example" - I did think we should do that, and then I sort of changed my mind, as they seem like semantically distinct things, and it seemed clearer therefore to represent them explicitly different. I'm very happy though that this PR is so small, so most of the same code is reused anyway :).

@wbamberg
Copy link
Collaborator

wbamberg commented Oct 1, 2019

Unfortunately this doesn't render the examples in "Applying color" as live samples.

The reason is https://github.com/mdn/stumptown-renderer/blob/master/client/src/ingredients/examples.js#L80 - the renderer uses the existence of the width property to decide whether this is a live sample or just some code blocks. If I add width: 800 to the example's front matter, then rebuild, it works perfectly :).

It's my fault: I omitted width in the examples for the "Applying_color" guide because I think we should perhaps always give live sample iframes full width. I talked about it in mdn/stumptown-content#54 (comment), and people seemed to agree, but we never properly confirmed it, and it was an oversight on my part that the renderer is still depending on this.

Relatedly, in mdn/stumptown-content#54 (comment), I discussed whether we should explicitly indicate whether an example is a static example or a live sample, rather than have the renderer use implicit hints like the existence of width. This issue suggests that we should :).

So, options now:

  1. we can merge this PR as-is, and I can update stumptown-content to include width.
  2. we can decide on a way forward like: (i) omit width, and have the renderer use the full article width for the iframe, then (ii) choose a syntax for indicating the example type (static or live-sample), and update both stumptown-content and stumptown-renderer accordingly.

We can also do both: (1) now, so we'll get guide pages rendering, and (2) in a follow-up. Having typed all this out, that's what I think we should do.

@joedarc
Copy link
Contributor Author

joedarc commented Oct 1, 2019

Wow good catch @wbamberg, i had a version of this that had all of those rendering, however i remember having to put a fallback to use 100% width. Let me know how you feel about this:

<RenderLiveSample example={example} /> remove the restriction to require width

<iframe
  className="live-sample-frame"
  srcDoc={srcdoc}
  title={example.description.title || "Live sample"}
  width={example.description.width || '100%'}
  height={example.description.height}
  frameBorder={0}
/>

Fallback to 100% width. Just checked and this renders them properly.

@joedarc
Copy link
Contributor Author

joedarc commented Oct 1, 2019

Scratch that, ignore me. I see what you mean. If you have any suggestions I'd be more than happy to change some stuff here.

@wbamberg
Copy link
Collaborator

wbamberg commented Oct 1, 2019

You could use height as the test instead of width, and use 100% if width is omitted. That way it ought to work for all live samples (height is definitely mandatory, at least I hope so...).

If we think 100% width all the time is the right way to go (I think I do), you could also always ignore width and just use 100%.

Then in a follow-up PR I could fix the content to add an explicit "example type" indicator.

@joedarc joedarc force-pushed the render-example-sections branch from a227224 to 115a721 Compare October 1, 2019 19:18
@joedarc
Copy link
Contributor Author

joedarc commented Oct 1, 2019

@wbamberg, I changed it. If the decision to make everything 100% is agreed upon i can change it to just use that all the time.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great. Thanks @joedarc , it's really exciting to see guide pages getting rendered!

@peterbe
Copy link
Contributor

peterbe commented Oct 2, 2019

I don't agree that this needs to wait for #156 because the only reason for waiting is to see that it worked. We can simulate that instead by deliberately putting in a chaos monkey that randomly throws an error.

@peterbe peterbe merged commit 29c0b3c into mdn:master Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with 'example' on /en-US/docs/Web/HTML/Applying_color
3 participants