-
-
Notifications
You must be signed in to change notification settings - Fork 312
Add char diff option #31
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
Conversation
praneshr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Go through the review comment and feel free to suggest.
| const computeDiff = (oldValue: string, newValue: string, useCharDiff: boolean): WordDiffInformation => { | ||
| const diffArray = diff | ||
| .diffChars(oldValue, newValue); | ||
| [useCharDiff ? 'diffChars' : 'diffWords'](oldValue, newValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be replaced with the following for better readability.
const diffArray = useCharDiff ? diff.diffChars(oldValue, newValue) : diff.diffChars(oldValue, newValue)| hideLineNumbers: false, | ||
| extraLinesSurroundingDiff: 3, | ||
| showDiffOnly: true, | ||
| useCharDiff: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value should be false else it will break the existing behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment compute-lines.ts is doing diff.diffChars(oldValue, newValue), which is the same as having true as default value for this option, or am I confusing something?
| |newValue |`string` |`''` |New value as string. | | ||
| |splitView |`boolean` |`true` |Switch between `unified` and `split` view. | | ||
| |disableWordDiff |`boolean` |`false` |Show and hide word diff in a diff line. | | ||
| |useCharDiff |`boolean` |`true` |Use char-based diff by default. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useCharDiff sounds confusing. What will happen when disableWordDiff: true and useCharDiff: true?
I would prefer to have one prop to control this. Say, diffType and it can take 3 possible values.
diffType: line|lineAndWord|lineAndChar
line: It will be the default behaviour. It shows the line-wise diff.
lineAndWord: It will show line and word-wise diff.
lineAndChar: It will show line and char-wise diff.
These three values can also be an enum. I prefer this over the previous method. For example,
export enum DiffType {
LINE = 0,
LINE_AND_WORD = 1,
LINE_AND_CHAR = 2,
}import React, { PureComponent } from 'react'
import ReactDiffViewer, {DiffType} from 'react-diff-viewer'
class Diff extends PureComponent {
render = () => {
return (
<ReactDiffViewer
diffType={DiffType.LINE_AND_CHAR}
oldValue={oldCode}
newValue={newCode}
splitView={true}
/>
)
}
}Suggestions are welcome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this approach too.
But I'm now somewhat confused about the defaults. Right now disableWordDiff is false by default. Which means that the diff is computed by using diff.diffChars by default. If I enable disableWordDiff — then neither diff.diffChars nor diff.diffWords is used, and the whole line is diffed. Which kind of implies that we need to enum this instead:
export enum DiffType {
LINE = 0,
WORD = 1,
CHAR = 2,
}
Is my assumption correct?
|
I'll put this PR on hold until this issue is solved #32 |
|
I'd love to use this feature too! |
|
Hey @bstst, the patch for the diff bug is released. Do you want to take a stab at this feature now? |
|
EDIT: I confirmed that this addresses my issue and created #51 that aims to accomplish what this PR is in a slightly different way. Would appreciate a review / any feedback! |
|
Outdated. |

I've seen this requested here #28, and now I find the need for this myself, so I'm suggesting this PR:
Adding a
useCharDiffoption, enabled by default, with which the diff works just the way it works in the latest build, so this not a breaking change.Once you do
useCharDiff={false}, it will calculate the diff based on words, not chars.