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(build): fix iframe title + div ids #11344

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix(build): fix iframe title + div ids #11344

wants to merge 5 commits into from

Conversation

fiji-flo
Copy link
Contributor

@fiji-flo fiji-flo commented Jun 20, 2024

Summary


How did you test this change?

Locally

- Don't mututate the DOM in collectClosestCode. We'll use the
  same title as with regual live samples.
- Strip suffix from iframe height height="400px" => height="400".
- Don't add ids to divs?! Currnet we add ids to notes by accident:
  https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_grid_layout/Masonry_layout#sect1
@fiji-flo fiji-flo requested a review from a team as a code owner June 20, 2024 15:40
@github-actions github-actions bot added the macros tracking issues related to kumascript macros label Jun 20, 2024
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 the warning message could be clearer.

Also: It would be nice for our translators to run a full build once, gather all the warnings, and put them as a checklist in an issue in translated-content, so that they have a chance to look at the affected live samples.

kumascript/src/api/util.ts Outdated Show resolved Hide resolved
kumascript/src/api/util.ts Show resolved Hide resolved
kumascript/macros/EmbedLiveSample.ejs Outdated Show resolved Hide resolved
@fiji-flo fiji-flo requested a review from caugner June 20, 2024 21:12
@fiji-flo fiji-flo changed the title fix(build): fix iframe title and height + div ids fix(build): fix iframe title + div ids Jun 20, 2024
@@ -16,7 +16,7 @@ <h2 id="Flexbox">Flexbox</h2>
and also tests that prerequisites are handled as part of the rendering
and live-sample-building process.
-->
<div id="Flex_1">{{ Page("Learn/CSS/CSS_layout/Introduction/Flex/Stuff", "Flex_1") }}</div>
<section id="Flex_1">{{ Page("Learn/CSS/CSS_layout/Introduction/Flex/Stuff", "Flex_1") }}</section>
Copy link
Contributor

Choose a reason for hiding this comment

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

@fiji-flo Do you know whether there are any occurrences of <div id="..."> outside of code blocks in content or translated-content that might have been added intentionally and are used to link to them?

Does rari check that the targets of anchor links are actually defined? I believe yari does not, but this would probably be useful, as links to anchors that don't exist are essentially dead links.

@github-actions github-actions bot added the idle label Jul 21, 2024
@github-actions github-actions bot removed the idle label Aug 16, 2024
Copy link
Contributor

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

@github-actions github-actions bot added merge conflicts 🚧 Please rebase onto or merge the latest main. and removed merge conflicts 🚧 Please rebase onto or merge the latest main. labels Aug 18, 2024
@github-actions github-actions bot added the idle label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idle macros tracking issues related to kumascript macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants