Support internal links, issue #95 #104
Conversation
|
Sorry I don’t have time to get to this today, but will try and take a deeper look sometime in the next few days. |
lib/fix-google-html.js
Outdated
| // normalize the tag name | ||
| if ( | ||
| ( | ||
| (node.tagName === 'b' || node.tagName === 'strong') | ||
| && (style['font-weight'] === 'normal' || style['font-weight'] === '400') | ||
| ) || | ||
| ( | ||
| (node.tagName === 'i' || node.tagName === 'em') && | ||
| style['font-style'] === 'normal' | ||
| ) | ||
| ) { | ||
| node.tagName = 'span'; | ||
| } |
There was a problem hiding this comment.
Is there a case you're hitting where Google Docs is outputting a <b>/<strong>/<i>/<em> tag and then setting the weight to not be bold/italic? I don't think I've seen this; curious about when it happens.
There was a problem hiding this comment.
I removed this since I could not find any clear examples of when/where this was needed in practice, and in the abstract, I think it's pretty debatable. Should the resulting markdown match the semantic info in the HTML or the styling in place? A <strong> could be restyled to not be bold, but instead be big, colored, underlined, or something else to show emphasis. It's not obvious that just because a <strong> is not bold it should not equate to **text** in markdown.
There was a problem hiding this comment.
I added that because the root element of the paste content is a bold element for some reason. However, that problem was resolved when I switched back to letting the browser handle the paste (instead of manually updating the innerHTML with the clipboard content on paste), so this is indeed obsolete. And I think you are right, the text might be bolder by default, and then this would break, so good call on removing it 👍
…lection independent of the clipboardData types order
if a b or strong element is actually not bold, we should not treat it as bold text
It's good to actually test the code! Especially since the new code does not pass its test now that we are actually running it. 😬
|
I went ahead and rebased this. I also added support for the GDocs Slice Clip data to I’ll have to do some more overall cleanup on this PR later in the week. |
This simplifies the `internal-links` test fixture a bit in order to keep it focused. This also removes some complicated markup around boldness in the headings because things get complicated there, and made a separate issue about it: Mr0grog#113. I've also moved the fixture to our official GDocs fixture folder.
The `<a>` element is meant for exactly this use case, not just clickable links (that's why it's called "a" -- short for "anchor"). This also moves it to the end of headings instead of the start, since that's where the existing test expectation has it. As a result, the `internal-links` test now passes.
Since headings now have anchors in them, other expectation fixtures that have headings need to include them, too. This makes unit tests pass.
|
OK, tests pass! This needed both some code changes and fixture changes. There is probably some more cleanup and tweaking worth doing here before merging, though. I also simplified the test fixture a bit, and removed the bold styling inside one of the headers, which it turns out behaves differently on different browsers. I’ve filed a separate issue to handle that: #113. |
The helper made code usefully concise if detailed error messaging wasn't needed (if you provide custom error handling, it doesn't buy you much). In most places it was used, I think we could have benefitted from better messaging (which I added) or we aren't actually conerned with the errors at all and can use constructs like optional chaining/optional calling to have even more concise code instead. There is one callsite that becomes notably worse, but I think only one is OK.
I could not find any clear examples of when/where this was needed in practice, and in the abstract, I think it's pretty debatable. Should the resulting markdown match the semantic info in the HTML or the styling in place? I think it's debatable; a `<strong>` could be restyled to not be bold but big, or colored, or underlined, or something else instead to show emphasis. It's not obvious that just because a `<strong>` is not bold it should not equate to `**text**` in markdown.
For errors and warnings, use `console.error|warn` directly so they don't get buried in hidden. For more debug/info level logs, continue using `debug` (and use the multiple argument form correctly, with formatting directives). I'm not super in love with `debug` in the browser, but it's already included by Micromark (used by Remark), so this is probably fine.
Oops, I should have done this as part of #104!
Oops, I should have done this as part of #104!
|
Thanks for the refactor! Lots of good calls! 🤩 Not sure about the change from |
Oh, this is literally what Obviously the ideal is to give the heading (or whatever element you are linking to) an |
Support internal links, resolves #95
I implemented it as per our discussions in #96