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.

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

Conversation

enagorny
Copy link
Contributor

@enagorny enagorny commented Nov 9, 2021

Summary

According to https://material.io/components/text-fields#interactive-demo (outlined, error)
Outlined TextInput should have bold border only when it is focused. And error state should change only color and not the boldness.

Test plan

existing

fixed

@callstack-bot
Copy link

callstack-bot commented Nov 9, 2021

Hey @enagorny, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@@ -385,7 +388,7 @@ const Outline = ({
{
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 })])
  );
});

@enagorny enagorny requested a review from lukewalczak November 10, 2021 17:29
);

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

Copy link
Member

@lukewalczak lukewalczak left a comment

Choose a reason for hiding this comment

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

Thanks @enagorny for fixing the bug and adjusting all comments!

@lukewalczak lukewalczak merged commit e509bf8 into callstack:main Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants