Skip to content

Conversation

@circAssimilate
Copy link
Contributor

@circAssimilate circAssimilate commented Nov 19, 2019

This PR is heavily inspired by #31, but differs slightly in it's approach. This is required functionality if we're going to be able to use react-diff-viewer in our product. Today, the default text diff comparison looks like this:

Screen Shot 2019-11-18 at 11 58 34 PM

But it would be more desirable for the implementer to be able to specify which JsDiff method is used, creating a more desirable diff in cases like this:

Screen Shot 2019-11-18 at 11 58 43 PM

Because the proposed jsDiffCompareMethod prop has a default value of diffChars, this would be a backwards compatible and a non-breaking change. disableWordDiff would still exist as a different prop and we'd simply return early if an unrecognized method name was passed.

In summary, this PR will:

  • Update the example app with the text diff method options
  • Add readme documentation
  • Add tests for the new functionality

Some other improvements worth considering in this or subsequent PRs:

Run this locally:
(NOTE: this node version works for me but someone should probably add .nvmrc with ideal node version)

# pull this branch / PR
nvm install 12.13.0
nvm use 12.13.0
yarn
yarn build:watch 

# In separate shell
nvm use 12.13.0
yarn start:examples

Totally open to feedback and very interested in helping push this over the line!

@circAssimilate circAssimilate force-pushed the circassimilate/add_text_diff_method_option branch from 1d3b224 to e53a882 Compare November 19, 2019 06:42
@praneshr
Copy link
Owner

Hi @circAssimilate, thanks for this. This is really useful. I'll take a look at this over the weekend. Thanks again.

@praneshr praneshr self-assigned this Nov 20, 2019
@praneshr praneshr added the v3.0.0 Features/fixes to be added in v3.0.0 label Nov 20, 2019
Copy link
Owner

@praneshr praneshr left a comment

Choose a reason for hiding this comment

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

LGTM. Can you check the PR comments and fix them. I'm planning to include this change in the upcoming v3.0

@circAssimilate
Copy link
Contributor Author

Thanks for the review, @praneshr. I'll aim to get to this today!

@circAssimilate
Copy link
Contributor Author

@praneshr, all ready for another review when you get the chance. 🎩👌

@circAssimilate
Copy link
Contributor Author

@praneshr, out of curiosity, do you have a rough timeline of when the 3.0.0 release will happen? We're doing some internal planning and I was hoping to communicate some expected timelines.

@praneshr
Copy link
Owner

praneshr commented Dec 3, 2019

@circAssimilate I'm so sorry for the delay. v3.0 will be out by Dec 8th. I'll merge your PR with my changes. Till then, you can use the forked module.

@circAssimilate
Copy link
Contributor Author

Sounds good, thank you!

@praneshr
Copy link
Owner

praneshr commented Dec 7, 2019

@circAssimilate Thanks for the contribution. Merging this to master. It will be released with v3

@praneshr praneshr merged commit 6075ff5 into praneshr:master Dec 7, 2019
@circAssimilate circAssimilate deleted the circassimilate/add_text_diff_method_option branch December 9, 2019 18:48
@circAssimilate
Copy link
Contributor Author

Thank you for the support and review, @praneshr!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v3.0.0 Features/fixes to be added in v3.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants