-
Notifications
You must be signed in to change notification settings - Fork 56
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
feat(Highlight): New Highlight Component #202
Conversation
This new component allows developers to highlight text based on a term condition
Replace the capital I for a lowercase i
* Fix visual tests by adding the visual snapshot * Add example on how to change the color of the highlight * Rename component folder from Hightlight to Highlight
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.
-
<Highlight text="Hello World!" highlight="!" />
renders the last span as an empty span, which can be avoided. -
<Highlight text="Hello World!" highlight="H" />
renders the first span as an empty span, which can be avoided. -
<Highlight text="A" highlight="a" />
renders three spans instead of a single one. -
<Highlight text="Hello World!" highlight="\Hello" />
highlightsHello
, which is not expected behavior I guess.
@bajcmartinez could you sign the license/cla? @dimabory Are you finished with your review? |
We should resolve this -> #202 (review) |
Avoid generating empty spans, and generate tests for the edge cases
@dimabory I've pushed new changes with test coverage for those cases and the resolution |
it('should render <Highlight> without trailing empty span', () => { | ||
const wrapper = shallow(<Highlight text="Hello World!" highlight="!" />); | ||
expect(wrapper.children()).toHaveLength(2); | ||
}); | ||
|
||
it('should render <Highlight> without leading empty span', () => { | ||
const wrapper = shallow(<Highlight text="Hello World!" highlight="H" />); | ||
expect(wrapper.children()).toHaveLength(2); | ||
}); | ||
|
||
it('should render <Highlight> a single span if no match', () => { | ||
const wrapper = shallow(<Highlight text="A" highlight="a" />); | ||
expect(wrapper.children()).toHaveLength(1); | ||
}); | ||
|
||
it('should render <Highlight> should escape \\', () => { | ||
const wrapper = shallow(<Highlight text="Hello World!" highlight="\world" />); | ||
expect(wrapper.children()).toHaveLength(1); | ||
}); |
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.
Could you add snapshots for these cases?
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.
Done
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.
Great job! 👍
Thank you @bajcmartinez
Types of Changes
Prerequisites
Please make sure you can check the following two boxes:
Contribution Type
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Description
This new component allows developers to highlight text based on a term
condition