Replaces all pre-existing h4 tags in notecards with <strong>

[tannerdolby]

Fixes #4394

Replaces h4s for notecards that have it, otherwise leaves them be.

Page in screenshot: http://localhost:3000/en-US/docs/Glossary/HTML5

[peterbe]

Great work getting the ball rolling!

[tannerdolby]

We love to see that green checkmark! tests are passing now, I'm not entirely sure what was holding them up. I had a few separate PRs that were failing late last night, in the "test dev server" suite.

[peterbe]

Oh I get it. The raw source doesn't have <h4 id="note">.
It gets injected by kumascript.

[tannerdolby]

Oh I get it. The raw source doesn't have <h4 id="note">. It gets injected by kumascript.

Ah, yeah that makes sense. Should we keep the id="note" applied to <strong> tags that are converted (if they have it) to maintain that ::before icon style or just force it to by the inline Note: some text like the majority of notecards are?

[tannerdolby]

After viewing https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch earlier today, I noticed that there are quite a few notecards with <h4 id="note"> specifically on this page. The logic you originally proposed removed the id if it had it and moved along transforming from <h4 id="note"> to <strong>. Since the id="note" is being injected by KS at some point, I bet it was intended somewhere along the line.

What do you think @schalkneethling ? Should an <h4 id="note"> keep its id when transformed to <strong id="note"> or be a clean slate and we can style the strong tags from there?

I understand that <h4>s will be removed completely in the "new notes" _original comment by @schalkneethling _. The h4's currently in notecards are being styled from the tagname selector and not actually using the #note selector at all in scss (from what I could find) which leads me to believe getting rid of id='note' completely won't be a problem.

[schalkneethling]

Should an <h4 id="note"> keep its id when transformed to <strong id="note"> or be a clean slate and we can style the strong tags from there?

Definitely just the clean strong. I bet we now have multiple instances of id="note" on a single page which is invalid HTML and removes any usefulness the id attribute might have had. Thank you for highlighting this @tannerdolby

[tannerdolby]

Definitely just the clean strong. I bet we now have multiple instances of id="note" on a single page which is invalid HTML and removes any usefulness the id attribute might have had.

Excellent, that is the current behavior, it transforms any <h4 id="note"> to plain <strong>. And your welcome @schalkneethling! I think the last thing that needs to be done here is creating a test fixture to see if everything is working as intended. Then this should be ready to merge.

Also, could you briefly explain how the fixtures should be created and tested/viewed? The last thread from Peter's review above explains where I'm at. I have the template /testing/.../check_notecards/index.html, and the test() started in index.test.js to be ran with npm run test:testing. Is this along the right direction?

[schalkneethling]

Also, could you briefly explain how the fixtures should be created and tested/viewed? The last thread from Peter's review above explains where I'm at. I have the template /testing/.../check_notecards/index.html, and the test() started in index.test.js to be ran with npm run test:testing. Is this along the right direction?

ping @peterbe - @tannerdolby I will have to learn along with you here :)

[peterbe]

Also, could you briefly explain how the fixtures should be created and tested/viewed? The last thread from Peter's review above explains where I'm at. I have the template /testing/.../check_notecards/index.html, and the test() started in index.test.js to be ran with npm run test:testing. Is this along the right direction?

My best tip is to go back to the .github/workflows/testing.yml and see how things are done there.

If you want to do regular jest tests of the builder, you type:

▶ export ENV_FILE=testing/.env

▶ yarn prepare-build && yarn build && yarn start:static-server

and then, in a new terminal type:

▶ yarn test:testing

[tannerdolby]

Just what I needed to get going. Thank you lots @peterbe !

I was able to setup a test fixture as I expected after following your proposed steps and looking at testing.yml.

[tannerdolby]

As expected, since we use p:first it works when

<div class="notecard note">
  <h4>foo</h4> 
   <p>one</p>
   <p>two</p>
</div>

but fails when there isn't a first-child <p> tag like

<div class="notecard warning">
<h4>foo</h4> 
some text
<p>two</p>
</div>

After trying out a bunch of different combinations for filtering text nodes from div.notecard, I arrived at a solution utilizing the nodeLabel property and specifically if the filtered text node collection had a length > 0 meaning they were text nodes otherwise we do our regular jam with p:first etc.

[tannerdolby]

Added some logic to handle the case of a <div class="notecard"> having a text node as the first adjacent sibling to the <h4> instead of the expected <p> tag. After much testing, this seems to be working nicely.

<div class="notecard note">
  <h4>Some heading</h4>
  No paragraph here
  <p>Paragraph 2</p>
</div>

<div class="notecard warning">
  <h4>Some heading</h4>
  <p>Paragraph 1</p>
  <p>Paragraph 2</p>
</div>

gets transformed to:

<div class="notecard note"><p><strong>Some heading:</strong> No paragraph here</p><p>Paragraph 2</p></div>

<div class="notecard warning"><p><strong>Some heading:</strong> Paragraph 1</p><p>Paragraph 2</p></div>

the .remove() chained on the filter function is whats stripping away all the whitespace as they are nodeType === 3 representing text nodes

[tannerdolby]

ping for review :) @schalkneethling

[schalkneethling]

I am happy with this work @tannerdolby - just want to see if @peterbe wants to take one more look.

[tannerdolby]

I am happy with this work @tannerdolby - just want to see if @peterbe wants to take one more look.

Ok sounds great @schalkneethling!

[peterbe]

Ideally, this should be a flaw. But I don't know if it's worth the effort because it's a lot of work.
Left some nits but otherwise, it seems to work great.

[tannerdolby]

🎉