Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added useEuiTextDiff hook #3288
Added useEuiTextDiff hook #3288
Changes from 7 commits
ed16415
2a84497
7c553dd
6c2a4b7
ad11dd4
4be06fd
c6cca37
295375d
0de0e0b
faa8259
2f798d8
5da5eb8
5c4e9a7
d8dca69
5f44db0
792775b
0760d25
51a41d5
cb7f3ef
4fce6e7
c27cc01
126f1f5
3896b5c
4869248
251ac01
cc81248
8c7e879
17bc834
058309d
cb2d2d2
b5b9ac3
fca53cb
1082578
0ca7ff3
85fc0ec
fee4259
231bdf7
7998222
3220821
a17b697
a5c343f
bb69275
b43effe
7f43118
a69e4c8
b135497
833bb0a
1b15f0b
85564c4
cbb4308
dd68031
8904525
c8ecd82
d9367f9
ae5f408
04218ea
fdf02c8
bbb5df6
de94ef8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think this example is better served with:
bigger diff, showing the effect of the timeout
more tuned values
example structure
moving the range slider to the top of the example
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'm worried about using our actual SASS vars to emulate a text diff. I think this could be confusing to users if they don't realize what page they've landed on. I also don't want to misconstrue any actual code that we ship. I'd much rather use fake content here.
We have a large text content block we use in both the scaling and text component docs that we could probably make a global constant for. I'd much rather use those than importing shippable code.
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.
Should we make separate file for
initialText
andcurrentText
specifically for this example ?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.
That would probably be fine!
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.
Okay 👍
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 think that change is too long; it'd likely cause too much lag while rendering (size of dom updates), and is just lengthy in terms of looking at an example. I'll keep thinking on this one.
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.
So, my current thoughts here are finding the right-sized diff from our JS->TS conversion process. @cchaos if we label the example as such would that work for you?
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.
yeah I think as long as the example has some sort of preamble about showing the diff of a JS to TS conversion, that would be fine.
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.
Actually I thought about it some more. My worry about using any code for any of these examples is that I think consumers will start thinking it should be as robust as a real code diff. Should it indicate white space changes, or look like a code editor with line numbers and such? If we're ok indicating that this could eventually be a full fledged code-diff, that's one thing. Otherwise, I'd stick to simple text.
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.
That's a great, future-thinking point. I'm gonna explore a little bit more before leaving any real thoughts here.