Skip to content
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.

Sign up for GitHub

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

fix: outlined input error border width #2975

Merged
merged 5 commits into from
Dec 10, 2021
Merged

fix: outlined input error border width #2975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
65a4050
fix: outlined input error border width
enagorny Nov 9, 2021
f8a35f1
test: outlined input error state
enagorny Nov 10, 2021
88f8027
test: explicitly test for borderWidth outline
enagorny Nov 11, 2021
2c49d31
test: borderWidth outline focused
enagorny Nov 14, 2021
eb04b16
test: better test name
enagorny Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions example/src/Examples/TextInputExample.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,13 @@ const TextInputExample = () => {
label="Custom rounded input"
/>
</View>
<View style={styles.inputContainerStyle}>
<TextInput
mode="outlined"
label="Outlined text input with error"
error
/>
</View>
</ScreenWrapper>
</TextInputAvoidingView>
);
Expand Down
7 changes: 6 additions & 1 deletion src/components/TextInput/TextInputOutlined.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ class TextInputOutlined extends React.Component<ChildTextInputProps> {
<Outline
theme={theme}
hasActiveOutline={hasActiveOutline}
focused={parentState.focused}
activeColor={activeColor}
outlineColor={outlineColor}
backgroundColor={backgroundColor}
Expand All @@ -314,6 +315,7 @@ class TextInputOutlined extends React.Component<ChildTextInputProps> {
labelBackground={LabelBackground}
/>
{render?.({
testID: 'text-input-outlined',
...rest,
ref: innerRef,
onChangeText,
Expand Down Expand Up @@ -365,6 +367,7 @@ export default TextInputOutlined;
type OutlineProps = {
activeColor: string;
hasActiveOutline?: boolean;
focused?: boolean;
outlineColor?: string;
backgroundColor: ColorValue;
theme: ReactNativePaper.Theme;
Expand All @@ -375,17 +378,19 @@ const Outline = ({
hasActiveOutline,
activeColor,
outlineColor,
focused,
backgroundColor,
}: OutlineProps) => (
<View
testID="text-input-outline"
pointerEvents="none"
style={[
styles.outline,
// eslint-disable-next-line react-native/no-inline-styles
{
backgroundColor,
borderRadius: theme.roundness,
borderWidth: hasActiveOutline ? 2 : 1,
borderWidth: focused ? 2 : 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @enagorny, could you please add snapshot testing that change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added snapshot test for outlined text input with error. Also added it to TextInputExample.
But I can't find a way how to properly set text input focused using react-native-testing-library.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukewalczak @enagorny what do you guys think about adding a testID in the Outline and then instead of a snapshot we can use testing-library to assert whether it has the right borderWidth?

Something like:

it('has the right borderWidth when the prop error is true', () => {
  const { getByTestID } = render(
    <TextInput
      mode="outlined"
      label="Outline Input with error"
      placeholder="Type Something"
      value={'Some test value'}
      error
    />
  );

  const outline = getByTestID('outline');

  expect(outline.props.style).toMatchObject({ borderWidth: 1 })
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brunohkbx, sounds good! @enagorny could you please adjust it to the suggestion above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukewalczak I'll try it.
Just thinking that adding testId might be out of scope of this PR. As it should be implemented for both text inputs and be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see it is suggested to add testId to outline only. Not the TextInput itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and still I can't test focused state 😞

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @enagorny, you can test focused state by:

  1. adding testID into TextInputOutlined eg text-input-outlined
  2. adding test scenario with fireEvent usage:
it('correctly applies focused state Outline TextInput', () => {
  const { getByTestId } = render(
    <TextInput
      mode="outlined"
      label="Outline Input with error"
      placeholder="Type Something"
      value={'Some test value'}
      error
    />
  );

  const outline = getByTestId('text-input-outline');
  fireEvent(getByTestId('text-input-outlined'), 'focus');
  expect(outline.props.style).toEqual(
    expect.arrayContaining([expect.objectContaining({ borderWidth: 2 })])
  );
});

borderColor: hasActiveOutline ? activeColor : outlineColor,
},
]}
Expand Down
42 changes: 41 additions & 1 deletion src/components/__tests__/TextInput.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import { StyleSheet } from 'react-native';
import { render } from 'react-native-testing-library';
import { fireEvent, render } from 'react-native-testing-library';
import TextInput from '../TextInput/TextInput';
import { red500 } from '../../styles/colors';

Expand Down Expand Up @@ -108,3 +108,43 @@ it('correctly applies height to multiline Outline TextInput', () => {

expect(toJSON()).toMatchSnapshot();
});

it('correctly applies error state Outline TextInput', () => {
const { getByTestId } = render(
<TextInput
mode="outlined"
label="Outline Input with error"
placeholder="Type Something"
value={'Some test value'}
error
/>
);

const outline = getByTestId('text-input-outline');
expect(outline.props.style).toEqual(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukewalczak @brunohkbx I've added testId to the outline so now it's possible to access outline props to check the style.
It's not possible to use toMatchObject here as style is array of styles of objects.
And because this project uses outdated React Native Testing Library version I can't setup jest-native to use toHaveStyle from it.
I end up writing this chunky matcher

expect.arrayContaining([expect.objectContaining({ borderWidth: 1 })])
);
});

it('correctly applies focused state Outline TextInput', () => {
const { getByTestId } = render(
<TextInput
mode="outlined"
label="Outline Input with error"
placeholder="Type Something"
value={'Some test value'}
error
/>
);

const outline = getByTestId('text-input-outline');
expect(outline.props.style).toEqual(
expect.arrayContaining([expect.objectContaining({ borderWidth: 1 })])
);

fireEvent(getByTestId('text-input-outlined'), 'focus');

expect(outline.props.style).toEqual(
expect.arrayContaining([expect.objectContaining({ borderWidth: 2 })])
);
});
2 changes: 2 additions & 0 deletions src/components/__tests__/__snapshots__/TextInput.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ exports[`correctly applies height to multiline Outline TextInput 1`] = `
},
]
}
testID="text-input-outline"
/>
<View
style={
Expand Down Expand Up @@ -392,6 +393,7 @@ exports[`correctly applies height to multiline Outline TextInput 1`] = `
],
]
}
testID="text-input-outlined"
underlineColorAndroid="transparent"
value="Some test value"
/>
Expand Down