Skip to content

fix: outlined input error border width#2975

Merged
lukewalczak merged 5 commits intocallstack:mainfrom
enagorny:outline-error-border-width
Dec 10, 2021
Merged

fix: outlined input error border width#2975
lukewalczak merged 5 commits intocallstack:mainfrom
enagorny:outline-error-border-width

Conversation

@enagorny
Copy link
Copy Markdown
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
Copy Markdown

callstack-bot commented Nov 9, 2021

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

backgroundColor,
borderRadius: theme.roundness,
borderWidth: hasActiveOutline ? 2 : 1,
borderWidth: focused ? 2 : 1,
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread src/components/__tests__/TextInput.test.js Outdated
Copy link
Copy Markdown
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