yari
yari copied to clipboard
feat(macros/EmbedInteractiveExample): use height from height-data.json
This is the second part of PR https://github.com/mdn/bob/pull/839 in BOB, which makes height-data.json
be created while building interactive examples. That file contains the height of every interactive example, so that height no longer needs to be provided from content
repository and we can also remove the height of each interactive editor type from here.
The height of the editor is now calculated by JS code in interactive-examples.tsx. During build, iframe of example is created, together with data-heights
attribute, which contains information about the height of the editor. This is an exemplary value:
[
{
"minFrameWidth": 0,
"height": 723
},
{
"minFrameWidth": 590,
"height": 654
}
]
The component will use the last height
, for which the value of minFrameWidth
was greater or equal to the calculated width of the iframe. This works just like media queries, but we don't use the width of the window in here, which gives incorrect results because of interference from the rest of the UI.
I changed the key of the Prose component to the following because the paragraph under iframe was causing duplicated react key exception(both having null value).
key={section.value.id || `top_level_prose${i}`}
If this gets merged, I will make the next PR to remove the second argument(height class) from every usage of EmbedInteractiveExample
in the content repository.
This PR depends on https://github.com/mdn/bob/pull/839 and shouldn't be merged until BOB update is deployed. Tests are failing because iframe cannot be shown, until #839 is merged
@caugner All of that sounds good, I will add a commit with those changes later.
I realized now that there is a better way to tackle this problem. Currently, heights are hardcoded in interactive-examples.scss in YARI and distributed across multiple CSS files in BOB. I could make height-data.json
send height in pixels instead of classes. A single record would look like that:
"pages/tabbed/audio.html": {
"landscape-height": "421px",
"portrait-height": "548px"
}
The file would get a bit bigger, but it would allow more complex height adjustments. For JavaScript examples, we could make height calculated by the formula BASE_HEIGHT + amountOfLines * LINE_HEIGHT
. It would make examples like Optional Chaining not be half empty.
Would you agree to that?
Edit: Since there was no reply for a long time, I have gone along with this, but JSON looks a bit different. I have updated the first post with the exact details.
This pull request has merge conflicts that must be resolved before it can be merged.
Hey @NiedziolkaMichal, a friendly ping. This one has a conflict that needs to be resolved. Thanks!
@schalkneethling It's done :) Tests seem to be failing because height-data.json
is not yet available.
This pull request has merge conflicts that must be resolved before it can be merged.
This pull request has merge conflicts that must be resolved before it can be merged.
I have rechecked everything and the code is working fine, but https://interactive-examples.mdn.mozilla.net/height-data.json
is still returning 404. While building interactive examples, this file exists and is available under the right location, but for some reason, it is not present in the production. I think that maybe "heightData": "./docs/height-data.json"
line is missing in the .bobconfigrc
in the production environment.
Hi @NiedziolkaMichal, I wonder if we couldn't just fix yari to listen to this height event:
https://github.com/mdn/bob/blob/077d0203a9c45b2a143a734169710c4ab49b4ce9/editor/js/editor-libs/events.js#L85-L92
I was thinking about it, but there is a significant drawback to this. Individual examples usually load 2-3 seconds after the rest of the content on my PC, where I have high-speed internet. If we send height information through an event, we will have a significant reflow after those few seconds, which would annoy the users. I was planning to use the event for examples that are not supported by the user's device because there is no way to predict this during the build, while currently a lot of space is being wasted.
This pull request has merge conflicts that must be resolved before it can be merged.
This pull request has merge conflicts that must be resolved before it can be merged.
Sorry for not addressing this in time. I see the issue, but we don't want to set height in JS here. We'll address this later this year.