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

fix(livesamples): use correct legacy url #9158

Merged
merged 4 commits into from
Jun 26, 2023
Merged

fix(livesamples): use correct legacy url #9158

merged 4 commits into from
Jun 26, 2023

Conversation

fiji-flo
Copy link
Contributor

Summary

We did not use the legacy URL in LiveSampleURL.ejs

Problem

Live samples translated from another page (deprecated), resulted in to 404 iframe.

Solution

Use the legacy url in LiveSampleURL.ejs to point to the legacy live sample.


Screenshots

Before

image

After

image


How did you test this change?

https://developer.mozilla.org/en-US/docs/Learn/Forms/How_to_build_custom_form_controls

With:

BUILD_LIVE_SAMPLES_BASE_URL=https://live.mdnplay.dev
BUILD_LEGACY_LIVE_SAMPLES_BASE_URL=https://live-samples.mdn.mozilla.net

Use a yarn build files/en-us/learn/forms/how_to_build_custom_form_controls/index.md and check that the iframes in the resulting html file have live-samples.mdn.mozilla.net as domain if they are not using the runner.html.

We did not use the legacy URL in LiveSampleURL.ejs
@fiji-flo fiji-flo requested review from caugner and LeoMcA June 26, 2023 08:39
@github-actions github-actions bot added the macros tracking issues related to kumascript macros label Jun 26, 2023
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit.

@fiji-flo fiji-flo requested a review from caugner June 26, 2023 09:07
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, but while you're cleaning up:


if ($1 && $1.length > 0) {
mdn.deprecatedParams("The second parameter of 'LiveSampleURL' is deprecated");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, the message would have an actionable suggestion on how to proceed. I guess in this case the original example has to be copied into the page, right?

var pagePath = env.url;
let pagePath = env.url;
let baseUrl = env.live_samples.base_url;
let samplePath = `${pagePath}/runner.html`.replace(/\/\/+/g, "/");

if ($1 && $1.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're cleaning up the macro anyways, can you assign $0 and $1 to variables id and url or similar to avoid referencing $0 and $1 below?

@fiji-flo fiji-flo merged commit 1da46c9 into main Jun 26, 2023
@fiji-flo fiji-flo deleted the fix-remote-samples branch June 26, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macros tracking issues related to kumascript macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants