Skip to content

Conversation

@wilsonpage
Copy link
Contributor

@wilsonpage wilsonpage commented Jul 10, 2019

Fixes #623

@vercel vercel bot temporarily deployed to staging July 10, 2019 17:45 Inactive
@vercel
Copy link

vercel bot commented Jul 10, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Copy link
Member

@johno johno left a comment

Choose a reason for hiding this comment

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

🏅

@ChristianMurphy
Copy link
Member

@wilsonpage would focusing the npm package to https://github.com/blakeembrey/param-case reduce the bundle size, and keep the edge case handling?

@johno
Copy link
Member

johno commented Jul 10, 2019

@ChristianMurphy at this point we shouldn't need any edge case handling since we're only transforming two known cases.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

This may not work for node.properties.data123 = '???'. DOM (and hast) represents data-123 as that, so there’s no uppercase to change. I’m guessing this change’ll break that. May need a test?

ARIA is a list of know values which don’t have numbers, so that’s fine.

@timneutkens
Copy link
Member

Updated the pr description to auto-close the issue.

@wilsonpage
Copy link
Contributor Author

@ChristianMurphy param-case still depends on the large dependency no-case.

@vercel vercel bot temporarily deployed to staging July 10, 2019 20:50 Inactive
@wilsonpage
Copy link
Contributor Author

Updated the PR to cope with the data123 case.

@vercel vercel bot temporarily deployed to staging July 10, 2019 21:00 Inactive
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Looks good

May need a test?

@wilsonpage any chance a test case could be added to verify the handling node.properties.data123?
So that use case is documented in code.

@wilsonpage
Copy link
Contributor Author

@ChristianMurphy I'm afraid I don't have enough context on this project to write a test. Would take me a while to grok, which I can't afford right now :( Would be great if one of you lot could add one!

@johno
Copy link
Member

johno commented Jul 11, 2019

I'll add a few tests in a follow up PR, thanks for the contribution @wilsonpage!

@johno johno merged commit eccea83 into mdx-js:master Jul 11, 2019
johno added a commit that referenced this pull request Jul 11, 2019
johno added a commit that referenced this pull request Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

The 'change-case' package is bloating bundle size

5 participants