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

create "underline <ins>" extension (#54) #60

Merged
merged 1 commit into from Sep 9, 2016

Conversation

pabranch
Copy link
Contributor

@pabranch pabranch commented Sep 5, 2016

copied from "strikethrough <del>" and renamed

Comments and suggestions are welcome.

@robinst
Copy link
Collaborator

robinst commented Sep 6, 2016

Hey there, welcome! This looks good already, I only have one comment:

I think we should call the artifact, package and class names ins instead. Whether it's rendered as underline or something else depends on the CSS that's used in the end, and is out of control of this library. A user of the library might not even render to HTML but just parse text with it. Note that the MDN entry about ins says nothing about ins being underline. We can keep the mentioning of underline in the descriptions. (I know this contradicts a bit with what the gfm-strikethrough extension does, but there it's because we follow exactly what GitHub does and calls it.)

We also require that you sign this CLA (only one time because it's your first contribution): https://na2.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=3f94fbdc-2fbe-46ac-b14c-5d152700ae5d

Once you've done the above change and sign the CLA, I'll gladly merge this :).

@pabranch
Copy link
Contributor Author

pabranch commented Sep 6, 2016

I am happy to make the name change. When I have a chance, I'll look over the CLA and send a new pull request. Thanks for such a prompt response!

@pabranch
Copy link
Contributor Author

pabranch commented Sep 6, 2016

How does this copy sound for the README.md?

Ins

Enables underline of text by enclosing it in ++. For example, in
hey ++you++, you will be rendered as underline text. Uses the <ins> tag.

Use class InsExtension in artifact commonmark-ext-ins.

@robinst
Copy link
Collaborator

robinst commented Sep 7, 2016

Sounds good! Don't forget to change the package name as well :).

copied from `commonmark-ext-gfm-strikethrough` and renamed
@pabranch
Copy link
Contributor Author

pabranch commented Sep 7, 2016

CLA has been signed. Let me know if you need me to email you a copy.

@robinst robinst merged commit 5a4f2fc into commonmark:master Sep 9, 2016
@robinst
Copy link
Collaborator

robinst commented Sep 9, 2016

Perfect, thank you! Will be included in the next release.

@pabranch pabranch deleted the add-underline-extension branch September 9, 2016 04:16
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.

None yet

2 participants