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 SmartyPants flag #5769

Merged
merged 6 commits into from
Jan 6, 2023
Merged

Add SmartyPants flag #5769

merged 6 commits into from
Jan 6, 2023

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Jan 5, 2023

Changes

Resolves #5752

SmartyPants was removed from Astro's defaults per #5684. This adds back our default with a markdown.smartypants option to disable.

Testing

Add SmartyPants tests for Markdown and MDX

Docs

@bholmesdev bholmesdev requested a review from a team as a code owner January 5, 2023 16:15
@changeset-bot
Copy link

changeset-bot bot commented Jan 5, 2023

🦋 Changeset detected

Latest commit: 9f0eb70

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) feat: markdown Related to Markdown (scope) labels Jan 5, 2023
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good!

.changeset/angry-pots-boil.md Outdated Show resolved Hide resolved
@matthewp
Copy link
Contributor

matthewp commented Jan 5, 2023

Were other naming choices discussed (I remember typographer being one)? Can you explain that decision?

@delucis
Copy link
Member

delucis commented Jan 5, 2023

Were other naming choices discussed (I remember typographer being one)? Can you explain that decision?

Seems like it would be odd to name it something different from the plugin we’re using? Unless we were planning to roll-our-own custom bundle of typography related plugins.

@matthewp
Copy link
Contributor

matthewp commented Jan 5, 2023

@delucis The reason would be so that you can swap out the implementation using a different plugin in the future, if there was a plugin that did the same thing better in some way.

I'm not advocating for a different name, I just remember there being discussions about what the name should be and think this is a good place to document the conclusions / reasonings for going with smartypants.

@matthewp
Copy link
Contributor

matthewp commented Jan 5, 2023

@bholmesdev can you mark this as closing #5752? thanks.

@@ -785,6 +785,23 @@ export interface AstroUserConfig {
* ```
*/
gfm?: boolean;
/**
* @docs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs approval here!

@bholmesdev
Copy link
Contributor Author

@matthewp Discussed this with @natemoo-re! In short, Goldmark is the only library I could find using "typographer" as their option name. "smartypants" has documentation across Python, Drupal, and JS Markdown libraries from an initial search. Sticking with smartypants makes sense for that reason.

@bholmesdev
Copy link
Contributor Author

@matthewp @natemoo-re Double checking what the version for this change should be? I chose "patch" instead of "minor" since it's a 2.0 beta feature.

@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Jan 6, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@bholmesdev bholmesdev merged commit 93e6339 into main Jan 6, 2023
@bholmesdev bholmesdev deleted the fix/add-smartypants-flag branch January 6, 2023 14:26
@sarah11918 sarah11918 mentioned this pull request Jan 24, 2023
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add smartypants back as a flag / option
5 participants