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

Adds height-data.json #839

Merged
merged 7 commits into from
Jan 26, 2023
Merged

Adds height-data.json #839

merged 7 commits into from
Jan 26, 2023

Conversation

NiedziolkaMichal
Copy link
Member

@NiedziolkaMichal NiedziolkaMichal commented Jul 20, 2022

This PR adds height-data.json, which is automatically generated based on interactive examples during the build process. That file contains information about the type of editor used by every interactive example. For example:

{
    "examples": {
        "pages/css/animation.html": "css",
        "pages/tabbed/sub.html": "tabbed-shorter",
        "pages/js/statement-switch.html": "js-taller"
    }
}

It will also include the content of editor-heights.json, added in this PR.
The file editor-heights contains name of every type of interactive editor, together with a height that should be dynamically set for it. For example:

{
  "editors": [
    {
      "name": "css",
      "heights": [
        {
          "minFrameWidth": 0,
          "height": 675
        },
        {
          "minFrameWidth": 590,
          "height": 375
        }
      ]
    },

This is height-data.json generated for latest interactive-examples.

Currently, the content repository must include information about height while adding interactive examples. For example:
{{EmbedInteractiveExample("pages/js/expressions-left-shift-assignment.html", "shorter")}}
That solution is flawed, as it requires height data specified in the content repo to exactly match the height specified in the interactive-example repo or height automatically decided by BOB. Changing the height of a single example requires two separate PRs and on multiple occasions height set in the content repo is incorrect(PR #18518).

Additionally, including exact information about the height of each editor, allows us to remove this data from YARI. Currently, the height set by YARI is on multiple occasions wrong, as it is calculating the height based on the viewport width, instead of the width of the interactive example. Setting heights only in BOB, allows us to make changes to the height freely, without making a second pull request in YARI, to match the changes.

This PR will build height-data.json together with interactive examples and make it accessible at https://interactive-examples.mdn.mozilla.net/height-data.json. PR #7679 in YARI uses this file to get the height of each interactive example, instead of taking information from EmbedInteractiveExample.

!!! This PR depends on #1004 which should be merged first. It moves the border of an editor from YARI to BOB.

@github-actions
Copy link
Contributor

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

@wbamberg
Copy link
Collaborator

Very nice! Is the idea here that heights for tabbed examples are still set manually using meta.json, but height for JS and WASM examples are set based on the line count?

@NiedziolkaMichal
Copy link
Member Author

The idea is to remove height from content repository.
It has following benefits:

  • If we rewrite HTML or other tabbed example and we want to change its height, we don't need to create additional PR in content repo.
  • After creating new example, we don't need to check its height and include it in PR in content repo.
  • If we would like to change how JavaScript and WAT examples are sized, we will simply need to change it in BOB. With current system, we would also need to check every single JS and WAT example in content and fix their sizes.

Generally I think current way of setting height for JS examples could be improved. IFrame sometimes is very big, even if half of the lines are empty: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/yield
I am planning to make some improvement to it and this PR would greatly help.

Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

This is a really useful addition and I'm excited to see this in the interactive examples!

@NiedziolkaMichal
Copy link
Member Author

@queengooborg Thank you. In the future, I plan to use this feature to fully fix #2053 and to make a lot more different sizes for JS/WAT editor, instead of just 3.

@github-actions
Copy link
Contributor

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

@bsmth
Copy link
Member

bsmth commented Jan 25, 2023

It looks like this is good to go. Is there anything else outstanding before we can merge this?

@NiedziolkaMichal
Copy link
Member Author

@bsmth From my side, everything is done

@wbamberg
Copy link
Collaborator

It looks like this is good to go. Is there anything else outstanding before we can merge this?

I was asked to review it but while I am very happy to review bob PRs from a product perspective (i.e. do they do things we want to support in interactive examples) I'm not comfortable reviewing nontrivial bob PRs from a code perspective, as I'm not really familiar with the code, and don't really want to be a code maintainer for it. I'm not sure who maintains bob any longer now it isn't @schalkneethling . But if y'all are happy with it, I don't want to block it.

@wbamberg wbamberg removed their request for review January 25, 2023 16:34
@queengooborg
Copy link
Collaborator

In that case, let's go ahead and merge this PR for the next Bob release!

@queengooborg queengooborg merged commit b4f2d09 into mdn:main Jan 26, 2023
@NiedziolkaMichal
Copy link
Member Author

@queengooborg Thanks for the review. I hope we won't have to wait long until this is actually deployed.

@queengooborg
Copy link
Collaborator

From what I can tell, we're just waiting for Claas to return, and then a new Bob version should be released soon!

@NiedziolkaMichal
Copy link
Member Author

That's good news. I will be looking forward to it :)

@queengooborg
Copy link
Collaborator

Oh hey @NiedziolkaMichal -- could you open up a PR to add MathML support to heightBuilder.js? I'm getting an error during the build due to the fact this was written before MathML support was introduced!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants