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

Add an em dash wrapper function #33

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

h3h
Copy link

@h3h h3h commented Apr 19, 2020

No description provided.

@avdgaag
Copy link
Owner

avdgaag commented Apr 21, 2020

Thanks for this PR! I’m not sure about this change; Smartypants will already replace --- with for you. If I’m writing an actual myself, I wouldn’t want a script to replace it with an HTML entity—I’d rather leave the .

Then again, there is an HTML entities function in there so this might be considered part of that (although I’d rather remove it), but then I wouldn’t expect a wrapping span tag around the .

@h3h
Copy link
Author

h3h commented Apr 21, 2020

🤷‍♂️ The span wrapping is to accomplish a proper typographic hair spacing around the em dash. I don't much care about the entity-vs-character aspect. I'd agree that the character is just fine on its own.

@avdgaag
Copy link
Owner

avdgaag commented Apr 22, 2020

If it’s about achieving the proper spacing, shouldn’t we replace the span tag with hair spaces, so that it outputs  —  (or  —  for that matter)?

@h3h
Copy link
Author

h3h commented Apr 25, 2020

Maybe, though the span gives the author control over the spacing via their stylesheet, as different fonts may render hair spaces in non-ideal proportions. (And also allows for authors to omit the spaces entirely if they wish.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants