search

eecs485staff / primer-spec

A Jekyll theme for sites with content-heavy pages
https://eecs485staff.github.io/primer-spec/
MIT License
22 stars 12 forks source link

Prevent newlines from being duplicated after copying an enhanced codeblock #248

Closed noah-weingarden closed 1 year ago

noah-weingarden commented 1 year ago

Closes #246. I wrote a test to expose this (and it's the first test for copying an enhanced codeblock in general). Unfortunately, jsdom doesn't implement innerText (see https://github.com/jsdom/jsdom/issues/1245), so I had to modify the map function to use textContent instead. I think that should be ok in this use case since the element containing the text for a single line of code is unlikely to ever have child nodes. If not though, I probably wrote that test for nothing.

github-actions[bot] commented 1 year ago

Thanks for opening this pull request! 🙏

I've changed this PR's base branch to develop. A maintainer will review your PR soon!

github-actions[bot] commented 1 year ago

The spec from this PR is available at https://preview.sesh.rs/previews/eecs485staff/primer-spec/248/.

(Available until Mon May 08 2023.)

seshrs commented 1 year ago

Thanks for putting this up @noah-weingarden! I’ll try to get to this over the weekend. Seems like we should just release a patch update asap to fix the bug.

I had to modify the map function to use textContent instead. I think that should be ok in this use case since the element containing the text for a single line of code is unlikely to ever have child nodes.

Actually, it’s likely for a single line of code to have child nodes since syntax highlighting is applied as span elements over specific tokens.

However, I agree with you that textContent is likely more appropriate for this use case. According to the MDN docs, textContent returns text from descendant nodes as well.

Could you update the unit test with more nested syntax highlighting nodes from an actual rendered code block? You should be able to get this by viewing the HTML source of a Primer Spec page.

noah-weingarden commented 1 year ago

Actually, it’s likely for a single line of code to have child nodes since syntax highlighting is applied as span elements over specific tokens. However, I agree with you that textContent is likely more appropriate for this use case. According to the MDN docs 1, textContent returns text from descendant nodes as well.

Haha, then textContent is appropriate for the exact opposite of why I thought it is. XD

Could you update the unit test with more nested syntax highlighting nodes from an actual rendered code block? You should be able to get this by viewing the HTML source of a Primer Spec page.

I don't think that would work: the event handler for clicking the button won't be attached if I just use the raw HTML for the rendered code block. We'd have to convert the HTML code to DOM nodes and then manually call copyLines instead of clicking the button. A couple downsides of that would be that 1) we'd need to export copyLines even though it makes more sense to be private to its module like it currently is and 2) the test would be less realistic at simulating a real user's interaction with the page. I guess another option would be to dynamically attach an event handler within the test, but that would still require extracting the event handler to be an exportable function, which feels a little arbitrary if it only needs to be exported to a test. Also, that would mean the test would be duplicating work that Primer Spec already does.

seshrs commented 1 year ago

I'm sorry I still haven't got to this. It's on my todo list for this weekend though!

noah-weingarden commented 1 year ago

Oh, I see what you meant now! And no problem!