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

Alternate n-dash handling #13

Closed
4 tasks done
sernaferna opened this issue Sep 27, 2024 · 12 comments
Closed
4 tasks done

Alternate n-dash handling #13

sernaferna opened this issue Sep 27, 2024 · 12 comments
Labels
💪 phase/solved Post is done

Comments

@sernaferna
Copy link

Initial checklist

Problem

For a context where smartypants is being used during active typing (i.e. it's being called on every character entered) it's impossible to get an m-dash because as soon as two -- are entered smartypants will change it to an n-dash before the third - can be entered.

Solution

I'd love the ability to replace two dashes with an n-dash but only if surrounded by spaces. I'm assuming it would become a new option for the dashes option, in addition to inverted and oldschool, because we don't to break all of the existing implementations.

These would be m-dashes; no spaces, so no n-dash replacement: Hello---my good friend!---How are you?

This would be an n-dash: Hey -- how's it going?

Alternatives

The alternate way to approach this would be to not actively call smartypants character-by-character during editing, in which case everything works as expected and hoped for and no changes are required. But I'm suggesting it in case it's an approach that others would also find helpful.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Sep 27, 2024
@wooorm
Copy link
Member

wooorm commented Sep 30, 2024

Could we do n-dash + - > m-dash?

@sernaferna
Copy link
Author

Could we do n-dash + - > m-dash?

Actually, yes, that would definitely work!

@wooorm
Copy link
Member

wooorm commented Sep 30, 2024

Want to work on a PR?

@sernaferna
Copy link
Author

Yes, I'll work on a PR! I've taken a look at the code and it seems doable; there's a solid base of test cases I can run (and add to) to make sure I haven't messed anything up. (Having not gotten into the details, I haven't yet discovered the devils. 😉) It probably won't happen quickly, but since I'm the one requesting it, I'm probably the only one waiting for it. 🙂

Given this suggestion, I'm tempted to leave the options for dashes alone, and simply modify the dashesOldSchool() function; thoughts? Any danger of breaking something else, given this approach?

@wooorm
Copy link
Member

wooorm commented Sep 30, 2024

Cool!

I don’t think this will break anyone. I think it makes sense that en-dash + dash are turned into em-dash. I don’t think it makes sense to want to keep them as-is.

@sernaferna
Copy link
Author

Cool!

I don’t think this will break anyone. I think it makes sense that en-dash + dash are turned into em-dash. I don’t think it makes sense to want to keep them as-is.

Agreed, so we've had at least two minds thinking about it. 😉

Thanks for your help! Will probably submit the PR sometime this week...

@sernaferna
Copy link
Author

Unfortunately, this change didn't turn out as easy as I'd hoped; @wooorm I think I need some more guidance.

It seems that the underlying libraries are separating different types of punctuation nodes, treating n-dash and m-dash characters separately from hyphens, so they result in separate invocations. For example,

function dashesOldschool(_, node) {
  console.log(`received '${node.value}' of type ${node.type}`)
  if (node.value === '–-') { // n-dash then hyphen
    node.value = '—'
  } else if (node.value === '---') {
    node.value = '—'
  } else if (node.value === '--') {
    node.value = '–'
  }
}

Notice the console.log; if I use the string started---typing–- (where that last part is an n-dash followed by a hyphen), this function gets called three times:

  • once for --- (as expected)
  • once for the n-dash in isolation
  • once for the hyphen in isolation

So it doesn't seem the function will ever receive the n-dash and hyphen together, to be able to replace them.

@wooorm
Copy link
Member

wooorm commented Oct 2, 2024

the function also gets, after node, an index and parent. You could use that to access the previous node: parent.children[index - 1]. Or parent.children[index + 1] for the next sibling.

More:

https://github.com/retextjs/retext-smartypants/blob/af8e7ec595442d552010a90b8b27a9498541482b/lib/index.js#L10C1-L16C1

What does get complex is that you’d want to remove a node. And that’s not supported yet. The return type of the functions is not yet used. It could start acting like a filter

@sernaferna
Copy link
Author

Hmm. I'll play with it, though if you can't remove a node, I don't have any brilliant ideas. 🤔

My use case is for MDXEditor, built on top of Lexical, so the good news is that I can actually add a transformer of my own that does the same thing, if I want to be selfish. But if I can make it work in retext-smartypants I do feel it would be useful for others, too, so I won't immediately give up...

@wooorm wooorm closed this as completed in f8f9683 Oct 10, 2024

This comment has been minimized.

@wooorm wooorm added the 💪 phase/solved Post is done label Oct 10, 2024
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Oct 10, 2024
@wooorm
Copy link
Member

wooorm commented Oct 10, 2024

Done in 6.2!

@sernaferna
Copy link
Author

Thanks @wooorm! Works great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

No branches or pull requests

2 participants