Added useEuiTextDiff hook by anishagg17 · Pull Request #3288 · elastic/eui

anishagg17 · 2020-04-09T17:25:06Z

Summary

stepping towards #1896

Created useEuiTextDiff component, hook based on text-diff which renders comparison of initial and final text to the user

Can take in timeout , customElement renderer as prop to enhance experience.

Screenshots

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in IE11 and Firefox
Props have proper autodocs
Added documentation examples
Added or updated jest tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

kibanamachine · 2020-04-09T17:25:08Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

anishagg17 · 2020-04-09T17:30:24Z

@miukimiu I don't know why euiColourSecondary isn't being applied to <ins> tag

at the same time euiColourDanger works for <del> tag

elizabetdev · 2020-04-09T17:49:41Z

jenkins test this

kibanamachine · 2020-04-09T18:11:20Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3288/

chandlerprall · 2020-04-22T17:06:31Z

To test, I've benchmarked diff, text-diff, fast-diff, and heckel-diff. The first three are all based on the algorithm presented in An O(ND) Difference Algorithm and Its Variations (Myers, 1986) while heckel-diff implements A technique for isolating differences between files (Heckel, 1978). Each library returns the same type of output, but heckel-diff is not as precise with its change boundaries.

benchmarks

two word difference in a one-line sentence
handfull of line deletions/modifications in a ~100 line file, simulating removing of a component's prop
replacing a 108 line TSX file with a 43 line SCSS file
adding a ~30 line comment to a 749 line TSX file

results
(times in ms, each test case+library combination ran 100 times)

case 0			avg	min	max
	diff		0	0	1
	textdiff	0	0	1
	fastdiff	0	0	1
	heckeldiff	0	0	1

case 1			avg	min	max
	diff		4	4	8
	textdiff	0	0	0
	fastdiff	0	0	0
	heckeldiff	1	1	3

case 2			avg	min	max
	diff		1363	1223	1964
	textdiff	41	37	86
	fastdiff	40	35	185
	heckeldiff	2	1	5

case 3			avg	min	max
	diff		336	319	511
	textdiff	0	0	0
	fastdiff	0	0	0
	heckeldiff	17	16	23

interpretations

diff's performance is terrible
heckel-diff is imperceptibly slower for a single diff in non-pathological cases, and substantially faster in pathological cases
text-diff and fast-diff average to the same, but text-diff has the faster warm-up/initial execution time

Given those results, my consideration is between text-diff and heckel-diff. Given that text-diff is faster in the general cases and has more accurate results, that library is my choice. It also happens to be the one already implemented in this PR. 🎉

Next I'll start reviewing the implementation of EuiTextDiff

chandlerprall

Thanks for your patience! I've run through and added some thoughts, but I really like how you've set it up.

src/components/code/_code_block.tsx

src/components/code/text_diff.tsx

anishagg17 · 2020-04-30T20:08:56Z

the text is updated when the timeout condition is met, it also triggers when timeout is changed but the initialText doesn't retain the state and that's why no further changes are observed on changing the slider

src/components/code/index.ts

src/components/code/text_diff.tsx

yarn.lock

src/components/code/_text_diff.scss

chandlerprall

Couple more things, and once Caroline's done with the design pass will take a last review of the docs side.

src/components/code/text_diff.tsx

src/components/code/text-diff.d.ts

chandlerprall · 2020-06-04T18:34:42Z

I've updated the Checklist in the description, remaining items are a changelog entry and couple jest snapshots:

snapshot using the default props
second snapshot with the three custom components
both snapshot tests should set timeout to 0 so we don't introduce any flakiness

/cc @cchaos if you want to take any more passes at this

cchaos · 2020-06-04T22:07:25Z

@anishagg17 My final review is PR4U: anishagg17#11

- Adding prop comments - Fixing text contrast - Adding snippets - Added “NEW” badge

cchaos · 2020-06-04T22:30:48Z

Oh and don't forget a changelog entry!

:wq :wqerge branch 'master' of https://github.com/elastic/eui into text

anishagg17 · 2020-06-05T09:40:03Z

I am getting such errors while rendering the test

cchaos · 2020-06-05T15:33:54Z

retest

kibanamachine · 2020-06-05T15:57:33Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3288/

anishagg17 · 2020-06-05T16:15:37Z

@cchaos I am really sorry to say that I forgot to push the snaps and updated test file. Now, the tests are working fine and snaps are updated.

cchaos · 2020-06-05T17:59:48Z

No worries! I'll rerun.

Jenkins, test this

kibanamachine · 2020-06-05T18:21:04Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3288/

chandlerprall

Woohoo! Changes LGTM!

cchaos

🎉 Awesome job! LGTM with a small CL change.

CHANGELOG.md

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

kibanamachine · 2020-06-05T21:00:34Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3288/

thompsongl · 2020-06-05T21:32:16Z

Congrats on getting this merged, @anishagg17 🎉

anishagg17 · 2020-06-05T21:35:52Z

Thank you very much 🚀

Added EuiTextDiff component

ed16415

fixed sass-linting

2a84497

chandlerprall self-assigned this Apr 17, 2020

chandlerprall suggested changes Apr 30, 2020

View reviewed changes

src/components/code/_code_block.tsx Outdated Show resolved Hide resolved

src/components/code/text_diff.tsx Outdated Show resolved Hide resolved

src/components/code/text_diff.tsx Outdated Show resolved Hide resolved

src/components/code/text_diff.tsx Outdated Show resolved Hide resolved

anishagg17 added 3 commits May 1, 2020 00:28

removed unwanted exports

7c553dd

improved Examples, exported all data from component

6c2a4b7

used useMemo

ad11dd4

chandlerprall suggested changes May 4, 2020

View reviewed changes

src/components/code/index.ts Outdated Show resolved Hide resolved

src/components/code/text_diff.tsx Outdated Show resolved Hide resolved

src/components/code/text_diff.tsx Outdated Show resolved Hide resolved

src/components/code/text_diff.tsx Outdated Show resolved Hide resolved

anishagg17 added 2 commits May 6, 2020 12:55

exported props,used for loop

4be06fd

allow user to pass components

c6cca37

chandlerprall suggested changes May 6, 2020

View reviewed changes

anishagg17 added 6 commits May 6, 2020 22:30

yarn.lock updated and deletion component used for example

295375d

used id as key, useMemos combined

0de0e0b

removed disableTimeout prop

faa8259

used useMemo for renderedHtml

2f798d8

Changed slider steps to display result

5da5eb8

tranformed to hook

5c4e9a7

cchaos reviewed May 7, 2020

View reviewed changes

src/components/code/_text_diff.scss Outdated Show resolved Hide resolved

removed padding , fontSize controll from euiText

d8dca69

chandlerprall suggested changes May 11, 2020

View reviewed changes

anishagg17 added 4 commits May 11, 2020 20:32

removed unnecessay props

5f44db0

used EuiTextDiffProps and removed Funcion def

792775b

used ElementType

0760d25

Used diff inside useMemo

51a41d5

Some design/docs additions (#11)

cbb4308

- Adding prop comments - Fixing text contrast - Adding snippets - Added “NEW” badge

anishagg17 added 3 commits June 5, 2020 14:42

Mt pull mine text

dd68031

:wq :wqerge branch 'master' of https://github.com/elastic/eui into text

Merge branch 'text' of https://github.com/anishagg17/eui into text

8904525

added test, CL, updated branch

c8ecd82

updated test

d9367f9

anishagg17 added 3 commits June 5, 2020 21:43

fixed snaps

ae5f408

snaps

04218ea

linting fixed

fdf02c8

chandlerprall approved these changes Jun 5, 2020

View reviewed changes

cchaos approved these changes Jun 5, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

anishagg17 and others added 2 commits June 5, 2020 18:40

Update CHANGELOG.md

bbb5df6

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

Merge branch 'master' into text

de94ef8

chandlerprall merged commit e03e961 into elastic:master Jun 5, 2020

anishagg17 deleted the text branch June 5, 2020 20:35

anishagg17 changed the title ~~Added EuiTextDiff component~~ Added useEuiTextDiff hook Jun 5, 2020

thompsongl mentioned this pull request Jun 29, 2020

Upgrade EUI to v26.3.1 elastic/kibana#70243

Merged

ohltyler mentioned this pull request Aug 28, 2020

26.0.0 causing Jest unit test import failures when rendering #3973

Closed

cchaos mentioned this pull request Sep 18, 2020

Text diff transition highlighting #1896

Closed

Conversation

anishagg17 commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

Checklist

Uh oh!

kibanamachine commented Apr 9, 2020

Uh oh!

anishagg17 commented Apr 9, 2020

Uh oh!

elizabetdev commented Apr 9, 2020

Uh oh!

kibanamachine commented Apr 9, 2020

Uh oh!

chandlerprall commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anishagg17 commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chandlerprall commented Jun 4, 2020

Uh oh!

cchaos commented Jun 4, 2020

Uh oh!

cchaos commented Jun 4, 2020

Uh oh!

anishagg17 commented Jun 5, 2020

Uh oh!

cchaos commented Jun 5, 2020

Uh oh!

kibanamachine commented Jun 5, 2020

Uh oh!

anishagg17 commented Jun 5, 2020

Uh oh!

cchaos commented Jun 5, 2020

Uh oh!

kibanamachine commented Jun 5, 2020

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Jun 5, 2020

Uh oh!

thompsongl commented Jun 5, 2020

Uh oh!

anishagg17 commented Jun 5, 2020

Uh oh!

anishagg17 commented Apr 9, 2020 •

edited

Loading

chandlerprall commented Apr 22, 2020 •

edited

Loading

anishagg17 commented Apr 30, 2020 •

edited

Loading