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

Rename ExpensiTextInput #6838

Merged
merged 6 commits into from
Jan 5, 2022
Merged

Conversation

mateusbra
Copy link
Contributor

Details

Rename component ExpensiTextInput to TextInput

Fixed Issues

$ #6760

Tests

  1. Run the app and see that TextInput still working

QA Steps

  1. Run the app and see that TextInput still working

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Web

Mobile Web

Mobile Web

Desktop

Desktop

iOS

IOS

Android

Android

@mateusbra mateusbra requested a review from a team as a code owner December 20, 2021 02:04
@MelvinBot MelvinBot requested review from parasharrajat and puneetlath and removed request for a team December 20, 2021 02:04
src/components/TextInput/BaseTextInput.js Outdated Show resolved Hide resolved
src/components/TextInput/BaseTextInput.js Outdated Show resolved Hide resolved
src/components/TextInput/BaseTextInput.js Outdated Show resolved Hide resolved
src/styles/styles.js Outdated Show resolved Hide resolved
@mateusbra
Copy link
Contributor Author

mateusbra commented Dec 20, 2021

We had a lint error due to having two style names that are the same:
textInput and textInput(expensiTextInput previously).
How you think we should rename it?

@parasharrajat
Copy link
Member

you can try baseTextInput if that works.

@puneetlath
Copy link
Contributor

It looks like we previously did it as RNText here:

import {Text as RNText} from 'react-native';

So maybe that's a good format to follow for when we import these components from React Native?

@mateusbra
Copy link
Contributor Author

mateusbra commented Dec 22, 2021

It looks like we previously did it as RNText here:

import {Text as RNText} from 'react-native';

So maybe that's a good format to follow for when we import these components from React Native?

You're saying we should change these imports:

import {
Animated, View, TouchableWithoutFeedback, Pressable,
} from 'react-native';

into:

import {
    Animated as AnimatedRN, 
View as ViewRN, 
TouchableWithoutFeedback as TouchableWithoutFeedbackRN, 
Pressable as PressableRN,
} from 'react-native';

Is that what you're proposing? @puneetlath

@puneetlath
Copy link
Contributor

Ah perhaps I've misunderstood. Can you help me understand why we need to call the files BaseExpensiTextInput instead of just TextInput?

@mateusbra
Copy link
Contributor Author

@puneetlath our file structure will be this:
Captura de Tela 2021-12-25 às 18 11 18
we won't call BaseExpensiTextInput, we will call BaseTextInput

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Small changes...

src/components/TextInput/index.js Outdated Show resolved Hide resolved
src/components/TextInput/index.js Outdated Show resolved Hide resolved
src/components/TextInput/index.native.js Outdated Show resolved Hide resolved
parasharrajat
parasharrajat previously approved these changes Dec 28, 2021
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM
cc: @puneetlath

puneetlath
puneetlath previously approved these changes Jan 4, 2022
Copy link
Contributor

@puneetlath puneetlath left a comment

Choose a reason for hiding this comment

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

@mateusbra thanks for your patience through the holidays. Everything looks good to me, but you've picked up some merge conflicts. Once you fix those I can quickly re-approve and merge. Thanks!

…> TextInput in AddressForm.js and AddDebitCardPage.js
@mateusbra mateusbra dismissed stale reviews from puneetlath and parasharrajat via 4047edb January 4, 2022 20:34
@puneetlath puneetlath merged commit 580c9db into Expensify:main Jan 5, 2022
@OSBotify
Copy link
Contributor

OSBotify commented Jan 5, 2022

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Jan 6, 2022

🚀 Deployed to staging by @puneetlath in version: 1.1.25-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.26-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants