Conversation
Preview URLs (140 pages)
Flaws (336)Note! 76 documents with no flaws that don't need to be listed. 🎉 URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
(comment last updated: 2025-02-21 13:59:28) |
| shape="poly" | ||
| coords="209,249,49,249,130,139" | ||
| href="https://developer.mozilla.org/docs/Web/JavaScript" | ||
| target="_blank" |
There was a problem hiding this comment.
FMI, How does the macro pick what code to include? Anything under the current heading? Including sub headings? I assume you are allowed just one HTML and one CSS?
There was a problem hiding this comment.
The macro picks up all interactive-examples code blocks in the current section. Multiple HTML/CSS/JS code blocks get merged with a newline.
FWIW Here's the logic: https://github.com/mdn/yari/pull/12633/files#diff-c74d432e1a65761aabd473c9e289fbb16c12d4ddc158ec626a2b90fb923ffb93R55-R82
hamishwillee
left a comment
There was a problem hiding this comment.
How do you want this reviewed?
- I compared about 30 images. They mostly differ only in a banner that is irrelevant to this (pity that couldn't be excluded). The ones with more significant differences still look good - the difference was things like the new version having video player controls on a video element that weren't present in the the original.
- I tried 10 pages that had been modified on the test server and they worked in the same way.
- Inspection of the code looks fine, but I didn't look at the original source and compare.
Upshot a spot check seems to verify this, but I'm not sure what would count as "acceptable review" for something like this?
|
@hamishwillee apologies, I should've expanded in my comment that I wasn't expecting a traditional review of this: we've done a bunch of automated testing to ensure that nothing's changed (that we don't want to change) already: so was expecting a smoke test much like what you've done - thank you! I think @bsmth will be having a look at this as well, and we'll wait until early next week to merge |
| </map> | ||
| <img | ||
| usemap="#infographic" | ||
| src="/shared-assets/images/examples/mdn-info.png" |
There was a problem hiding this comment.
Are we doing some magic for https://mdn.github.io prefixes?
Works for me locally, so I presume so:
There was a problem hiding this comment.
Yes, we proxy /shared-assets/ to https://mdn.github.io/shared-assets/
There was a problem hiding this comment.
@LeoMcA Remind me again, why don't we replace the links with a fully qualified link to e.g. https://mdn.github.io/shared-assets/images/diagrams/http/overview/fetching-a-page.svg? It doesn't work in all cases? (The big advantage is that beginners can copy the code and it will just work.)
There was a problem hiding this comment.
beginners can copy the code and it will just work
This is nice, too, and worth taking into consideration. There aren't many proxy link instances (it's in 19 files), so I don't think it's critical -> changing this later doesn't have a wide impact.
There was a problem hiding this comment.
A couple of reasons:
- not all examples work with a fully qualified link because of cross origin issues:
<track>is the example which demonstrated this, and it wouldn't be easily possible to figure out which others don't work without manually testing them one by one - it makes the example code much longer, and space is already limited within the editors here
I'm not really convinced by the need to make these run without modification after copying: the point of the interactive example is to allow users to edit the code in-page, not locally or elsewhere
bsmth
left a comment
There was a problem hiding this comment.
Everything looks good, tnx. Discussion about the proxy URLs is not blocking, although Claas has a convincing opinion there.
Thanks a lot!
<!-- 🙌 Thanks for contributing to MDN Web Docs. Adding details below will help us to merge your PR faster. --> ### Description correct a little wrong usage ### Motivation <!-- ❓ Why are you making these changes and how do they help readers? --> ### Additional details <!-- 🔗 Link to release notes, vendor docs, bug trackers, source control, or other places providing more context --> ### Related issues and pull requests <!-- 🔨 If this fully resolves a GitHub issue, use "Fixes mdn#123" --> <!-- 👉 Highlight related pull requests using "Relates to mdn#123" --> <!-- ❗ If another pull request should be merged first, use "**Depends on:** mdn#123" --> <!-- 👷♀️ After submitting, go to the "Checks" tab of your PR for the build status -->
Migrate HTML Interactive Examples (#38257) <!-- 🙌 Thanks for contributing to MDN Web Docs. Adding details below will help us to merge your PR faster. --> ### Description correct a little wrong usage ### Motivation <!-- ❓ Why are you making these changes and how do they help readers? --> ### Additional details <!-- 🔗 Link to release notes, vendor docs, bug trackers, source control, or other places providing more context --> ### Related issues and pull requests <!-- 🔨 If this fully resolves a GitHub issue, use "Fixes #123" --> <!-- 👉 Highlight related pull requests using "Relates to #123" --> <!-- ❗ If another pull request should be merged first, use "**Depends on:** #123" --> <!-- 👷♀️ After submitting, go to the "Checks" tab of your PR for the build status -->
Migrate HTML Interactive Examples (mdn#38257) <!-- 🙌 Thanks for contributing to MDN Web Docs. Adding details below will help us to merge your PR faster. --> ### Description correct a little wrong usage ### Motivation <!-- ❓ Why are you making these changes and how do they help readers? --> ### Additional details <!-- 🔗 Link to release notes, vendor docs, bug trackers, source control, or other places providing more context --> ### Related issues and pull requests <!-- 🔨 If this fully resolves a GitHub issue, use "Fixes mdn#123" --> <!-- 👉 Highlight related pull requests using "Relates to mdn#123" --> <!-- ❗ If another pull request should be merged first, use "**Depends on:** mdn#123" --> <!-- 👷♀️ After submitting, go to the "Checks" tab of your PR for the build status -->

See https://github.com/orgs/mdn/discussions/782 for further explanation.
This PR migrates the HTML examples, see the results on https://test.developer.allizom.org/en-US/
Also see the results of the visual diff: en-US.zip
The differences here (highlighted in red) are expected due to some small changes to the default interactive examples styles.