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

Live samples have a reduced height and get scrollbars #6445

Closed
wbamberg opened this issue Jun 2, 2022 · 6 comments
Closed

Live samples have a reduced height and get scrollbars #6445

wbamberg opened this issue Jun 2, 2022 · 6 comments
Labels
live-samples issues related to live samples (EmbedLiveSample macro)

Comments

@wbamberg
Copy link
Collaborator

wbamberg commented Jun 2, 2022

I can't find an existing issue about this, apologies if there is one.

I think this might be the result of an earlier change to add a border to live samples: #3260 . This stole some usable height from the iframe, and as a result all examples whose content was close to occupying the max height (which is a lot of them) got scrollbars, which is ugly and makes it harder to scroll the page. In many cases the output is also cut off. For example, here's the output for the example at https://developer.mozilla.org/en-US/docs/web/css/@font-face#examples:

Screen Shot 2022-06-01 at 9 15 36 PM

We could fix this one at a time in content (and I'm about to fix this one) but it would be much better to have a platform fix.

@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Jun 2, 2022
@OnkarRuikar
Copy link
Contributor

PR #5337 added 1rem padding to the sample-code-frame class. If we increase the height by 32px then there is no scrollbar. The platform can add 32px to the iframe height. But it'll also affect the places where we've already manually increased the heights to get rid of the scrollbar. Extra 32px height won't affect much in such cases.

I suggest we update the mdn/content at once and not worry about it again.
We could search and replace(add 32px) to all the macros in mdn/content. 🤔
For good measure we can include the 1px border and make it 34px or even higher.

@wbamberg
Copy link
Collaborator Author

wbamberg commented Jun 2, 2022

PR #5337 added 1rem padding to the sample-code-frame class. If we increase the height by 32px then there is no scrollbar. The platform can add 32px to the iframe height. But it'll also affect the places where we've already manually increased the heights to get rid of the scrollbar. Extra 32px height won't affect much in such cases.

I don't care much if example iframes are 32px taller than they need to be.

I suggest we update the mdn/content at once and not worry about it again. We could search and replace(add 32px) to all the macros in mdn/content. 🤔 For good measure we can include the 1px border and make it 34px or even higher.

We could fix it in content, yes, but it seems like it would be much less work, and much safer, to fix in the platform.

@OnkarRuikar
Copy link
Contributor

I don't care much if example iframes are 32px taller than they need to be.

Agreed. But the future contributors may wonder why do they have to set 32px less in the macro in order to get the exact height. We document it or not it'll remain a strange fact about the platform.

it would be much less work

Yes, it is a considerable amount of work to update the content. At the moment there are ~2481 EmbedLiveSample macros in mdn/content. It can be automated. But the reviews will take time because the reviewer will have scan the outputs using the PR companion.

@teoli2003
Copy link
Contributor

teoli2003 commented Jun 6, 2022

I wonder if Yari can compute the correct height (at least in 99% of the cases) when building the page. This wasn't possible in the wiki-era.

It would simplify writers' work and prevent such problems. We could then deprecate the parameter (or set it mostly to "" if there are edge cases.

cc/ @mdn/core-dev to know if this would be the correct answer to this problem, or not.

Maybe it doesn't need to be an <iframe> anymore, too! This may have been a security requirement from the wiki era that is no more needed.

@schalkneethling
Copy link

@teoli2003 We used to set different classes, which set different heights, on the interactive examples. This was done using a postMessage from the editor to the parent page if I remember correctly. Perhaps we are no longer doing this? It could also be that we are, but that with the redesign some ratios changes and we need to update those heights to accommodate the changes. Let’s see what the engineering folks say. Thank you for the ping.

@caugner
Copy link
Contributor

caugner commented Sep 12, 2022

This was fixed in #6506, and will be further improved by #7079.

For now, we don't plan to compute the correct height automatically, as it would most certainly increase our build time.

@caugner caugner closed this as completed Sep 12, 2022
@caugner caugner added live-samples issues related to live samples (EmbedLiveSample macro) and removed needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. labels Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
live-samples issues related to live samples (EmbedLiveSample macro)
Projects
None yet
Development

No branches or pull requests

5 participants