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

Split TextInput into TextInputOutlined and TextInputFlat #882

Merged
merged 4 commits into from
Mar 19, 2019

Conversation

pbitkowski
Copy link
Contributor

What this PR consists of:

  • make TextInput proxy component that renders TextInputOutlined or TextInputFlat based on 'mode' props,
  • move TextInput components to its own directory and change imports in index to make it work.

Motivation

Issue #791

Test plan

Open example and compare it with version without my changes.

@callstack-bot
Copy link

callstack-bot commented Mar 1, 2019

Hey @pbitkowski, thank you for your pull request 🤗. The documentation from this branch can be viewed here. Please remember to update Typescript types if you changed API.

@pbitkowski
Copy link
Contributor Author

I have an issue with Flow here. It logs a long list of errors connected to possible usage of TextInput (from React Native) props on our TextInput. I used the same Properties as before so it's weird that it doesn't work. I'm not sure if the intersaction of React.ElementConfig and Object exact type is correct in TextInput props. Could you check it @ferrannp ? Thanks :)

@satya164
Copy link
Member

satya164 commented Mar 2, 2019

I think the state can be moved up to the TextInput component to reduce duplication. Let's also move the types to a common file. The documentation can be removed from the individual components as well.

For flow types, try spreading them instead of intersection:

type Props = {|
  ...React.ElementConfig<typeof NativeTextInput>,
|}

@pbitkowski
Copy link
Contributor Author

@satya164 I moved types to common file and used spread operator for extending React Native TextInput Props but still got errors, which is weird. BTW. I rebased my branch to master and some TS errors appeared.

@ferrannp ferrannp requested review from satya164 and removed request for ferrannp March 7, 2019 10:39
@pbitkowski
Copy link
Contributor Author

Fixed flow errors and rebased. Could you take a look @Trancever ?

error: false,
multiline: false,
editable: true,
render: props => <NativeTextInput {...props} />,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we type it as RenderProps just like in other components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


return (
<View style={[containerStyle, style]}>
{/* When mode === 'flat', render an underline */}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


{mode === 'outlined' && label ? (
{label ? (
// When mode == 'outlined', the input label stays on top of the outline
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove When mode == 'outlined from comment :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

- make TextInput proxy component that renders TextInputOutlined or TextInputFlat based on 'mode' props,
- move TextInput components to its own directory and change imports in index to make it work.
- move types from TextInput, TextInputFlat, TextInputOutlined to common file,
- remove documentation from TextInputFlat and TextInputOutlined
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.

None yet

5 participants