yari icon indicating copy to clipboard operation
yari copied to clipboard

feat(macros/EmbedInteractiveExample): use height from height-data.json

Open NiedziolkaMichal opened this issue 2 years ago • 11 comments

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

NiedziolkaMichal avatar Nov 26 '22 22:11 NiedziolkaMichal

@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.

NiedziolkaMichal avatar Nov 28 '22 21:11 NiedziolkaMichal

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Dec 16 '22 09:12 github-actions[bot]

Hey @NiedziolkaMichal, a friendly ping. This one has a conflict that needs to be resolved. Thanks!

schalkneethling avatar Jan 16 '23 09:01 schalkneethling

@schalkneethling It's done :) Tests seem to be failing because height-data.json is not yet available.

NiedziolkaMichal avatar Jan 18 '23 23:01 NiedziolkaMichal

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Jan 23 '23 14:01 github-actions[bot]

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Feb 08 '23 12:02 github-actions[bot]

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.

NiedziolkaMichal avatar Feb 25 '23 13:02 NiedziolkaMichal

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

caugner avatar Mar 08 '23 13:03 caugner

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.

NiedziolkaMichal avatar Mar 08 '23 13:03 NiedziolkaMichal

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Mar 15 '23 21:03 github-actions[bot]

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Mar 13 '24 12:03 github-actions[bot]

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.

fiji-flo avatar Mar 28 '24 15:03 fiji-flo