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

Bring lib inline with react 0.14 #8

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

Conversation

davnicwil
Copy link
Collaborator

This brings the min peer dependency up to react 0.14 (the react and new react-dom modules are now both depended upon @ 0.14.x).

No functionality changes, but ensures everything's in line with 0.14 so the library will now work up to at least react 0.16, and allowed for one code/perf improvement under the hood.

Does mean library will no longer work for any consumers using react < 0.14, so have bumped the major version number to 1.x.x


Changes:

  • Replaced ES6 class with new react 0.14 stateless functional component pattern. Less code, and promises performance optimisations in future versions.
  • Fixed tests which failed against 0.14 - react 0.14 made some fixes/improvements to generated html, so expected markup is now slightly different.
  • Upgraded function calls so all now-deprecated ones are no longer called. Involved use of new react-dom module in the tests.
  • Introduced change to eliminate warning introduced in react 0.14 about img tags having children. Summary: children array was being passed generically for all elements, was empty for img tag but React still complained. Probably didn't matter anyway, but better to address it and not have the warning.

… style prop - previously committed test now passes. Bump minor version number to indicate new, backwards-compatible feature.
… peer dependencies to 0.14.x. Address react warnings by modifying createElement call to not pass children to img tags. Update deprecated function calls. Fix tests to expect different rendered markup from react 0.14.x
…y extra provided props through to the root element, instead of only supporting specified ones.
@davnicwil
Copy link
Collaborator Author

Again, this branch incorporates the root element props features in my other pull request.

I guess it might make sense to merge that one first, then create a 0.x branch before pulling this, so react > 0.13.2 support still continues on its own legacy branch.

@thangngoc89
Copy link

@davnicwil why did you remove lib folder from .gitignore ? It will be published to npm anyway ( https://github.com/alexkuz/markdown-react-js/blob/master/.npmignore )

@davnicwil
Copy link
Collaborator Author

@thangngoc89 it's because I'm currently requiring the library like this:
dependencies: {
"markdown-react-js" : "davnicwil#1.x"
}

I.e. pulling from github rather than the npm central repo since my branch isn't yet merged, built, put into npm central repo etc.

So before, without the build in lib/index.js in the repo, after pulling in the library I had go to node_modules, copy in the source and build it manually. Now it just works.

@thangngoc89
Copy link

Thank you for clarification. Did you heard from the author recently ?

@davnicwil
Copy link
Collaborator Author

No worries! Last I heard @alexkuz told me he was reviewing my PRs, as well as considering other changes.

Any updates on this @alexkuz? Would be handy to get these changes into npm central :-)

@alexkuz
Copy link
Owner

alexkuz commented Feb 6, 2016

@davnicwil sorry, I still haven't reviewed these.

As for lib, it's better to leave it in .gitignore, I believe (you can use npm link ~/your-projects-dir/markdown-react-js for development).

@kof
Copy link
Collaborator

kof commented Oct 18, 2017

Is this fixed in 0.4.0?

@kof
Copy link
Collaborator

kof commented Dec 12, 2018

hey @davnicwil would you like to be added as a maintainer?

@davnicwil
Copy link
Collaborator Author

@kof I haven't looked at this in a long time, and likely won't be able to give it much attention until after the new year, but sure if you want to add me I would be happy to clean this up and get it merged.

@alexkuz
Copy link
Owner

alexkuz commented Dec 18, 2018

I've sent invites to @davnicwil and @smm-telus. Thank you everyone!

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

4 participants