feat: implement highlightedtext component #1864
Conversation
Features
Contributors@amontalvof, @TahimiLeonBravo, @LeandroTorresSicilia Commit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| import styled from 'styled-components'; | ||
| import attachThemeAttrs from '../../../styles/helpers/attachThemeAttrs'; | ||
|
|
||
| // eslint-disable-next-line import/prefer-default-export |
There was a problem hiding this comment.
remove the eslint exception
| import attachThemeAttrs from '../../../styles/helpers/attachThemeAttrs'; | ||
|
|
||
| // eslint-disable-next-line import/prefer-default-export | ||
| export const HighlightedContainer = attachThemeAttrs(styled.div)` |
There was a problem hiding this comment.
If we aren't adding some styles to the container, we would not create a styled for that
|
@amontalvof also we need to export the new components on the |
src/components/HighlightedText/__test__/HighlightedText.spec.js
Outdated
Show resolved
Hide resolved
| .html() | ||
| .includes('Honeycrisp'), | ||
| ).toBe(true); | ||
| }); |
There was a problem hiding this comment.
we should change these tests to make clearer the difference between a hit and normal text
There was a problem hiding this comment.
this second test you can delete the first the third test all the code
| ##### This example shows the style that is applied by default to the component. | ||
| ```js | ||
| import React, { useState } from 'react'; | ||
| import { HighlightedText } from 'react-rainbow-components'; |
| { | ||
| "value" : "Apples come in several ", | ||
| "type" : "text" | ||
| }, | ||
| { | ||
| "value" : "varieties", | ||
| "type" : "hit" | ||
| }, | ||
| { | ||
| "value" : ", including Fuji, Granny Smith, and Honeycrisp.", | ||
| "type" : "text" | ||
| } |
| # HighlightedText base | ||
| ##### This example shows the style that is applied by default to the component. | ||
| ```js | ||
| import React, { useState } from 'react'; |
There was a problem hiding this comment.
useState is not used in the example
| ##### This example shows the component when custom styles are applied to it. | ||
|
|
||
| ```js | ||
| import React, { useState } from 'react'; |
| ```js | ||
| import React, { useState } from 'react'; | ||
| import styled from 'styled-components'; | ||
| import { HighlightedText } from 'react-rainbow-components'; |
| { | ||
| "value" : "Apples come in several ", | ||
| "type" : "text" | ||
| }, | ||
| { | ||
| "value" : "varieties", | ||
| "type" : "hit" | ||
| }, | ||
| { | ||
| "value" : ", including Fuji, Granny Smith, and Honeycrisp.", | ||
| "type" : "text" | ||
| } |
| const TextContainer = styled.span` | ||
| color: gray; | ||
| font-size: 1rem; | ||
| `; | ||
|
|
||
| const HitContainer = styled.span` | ||
| background-color: #00a3dc; | ||
| color: #fff; | ||
| font-size: 1rem; | ||
| `; |
There was a problem hiding this comment.
I think it would be better to use the theme colors for this example
| .html() | ||
| .includes('Honeycrisp'), | ||
| ).toBe(true); | ||
| }); |
There was a problem hiding this comment.
this second test you can delete the first the third test all the code
| expect( | ||
| container | ||
| .at(0) | ||
| .html() | ||
| .includes('Apples'), | ||
| ).toBe(true); | ||
| expect( | ||
| container | ||
| .at(1) | ||
| .html() | ||
| .includes('varieties'), | ||
| ).toBe(true); | ||
| expect( | ||
| container | ||
| .at(2) | ||
| .html() | ||
| .includes('Honeycrisp'), | ||
| ).toBe(true); |
There was a problem hiding this comment.
I think it would be cleaner like this
const component = mount(<HighlightedText parts={parts} />);
const container = component.find('span');
expect(container.length).toBe(3);
parts.forEach(({ value }, index) => {
expect(
container
.at(index)
.html()
.includes(value),
).toBe(true);
});
| `; | ||
|
|
||
| // eslint-disable-next-line react/prop-types | ||
| const hitComponent = ({ children }) => { |
There was a problem hiding this comment.
capitalize the first letter HitComponent
| @@ -0,0 +1,104 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
HighlightedText.spec.js change to highlightedText.spec.js
| export interface HighlightedText extends BaseProps { | ||
| hitComponent?: ComponentType<{ children?: string }>; | ||
| textComponent?: ComponentType<{ children?: string }>; | ||
| part?: Part[]; |
There was a problem hiding this comment.
it is parts:
parts?: Part[];
| * HighlightedText is a component that highlights a part of a text. | ||
| */ | ||
|
|
||
| function HighlightedText(props) { |
There was a problem hiding this comment.
when using function declaration you can export directly here like:
export default function HighlightedText(props) {...
| maxWidth:"700px", | ||
| textAlign:"center", | ||
| padding:"20px", | ||
| margin:"auto", |
There was a problem hiding this comment.
use single quotes and add a space after the : symbol like:
maxWidth: '700px',
| export default HighlightedText; | ||
|
|
||
| HighlightedText.propTypes = { | ||
| /** The class of the component. */ |
There was a problem hiding this comment.
In order to be consistent, we should use the same description that we use in other components
A CSS class for the outer element, in addition to the component's base classes.
| const finalTextContainer = textComponent || DefaultTextContainer; | ||
|
|
||
| return ( | ||
| <p className={className} style={style}> |
There was a problem hiding this comment.
We need to add a new prop isInline in order to change the p element by a span element, also we should add a useful description for this prop
There was a problem hiding this comment.
add a new example using the isInline prop
…com/nexxtway/react-rainbow into implement-highlightedtext-component
|
Code Climate has analyzed commit 45634f4 and detected 2 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
fix: #1847
Changes proposed in this PR:
I have followed (at least) the PR section of the contributing guide.
@nexxtway/react-rainbow