Skip to content

Conversation

@mateusbra
Copy link
Contributor

Details

Rename component ExpensiPicker to Picker, rename folder Picker to NativePicker in order to solve import conflicts

Fixed Issues

$ #6760

Tests

  1. Run the app and see that Picker still working

QA Steps

  1. Run the app and see that Picker 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 19, 2021 23:15
@MelvinBot MelvinBot requested review from parasharrajat and puneetlath and removed request for a team December 19, 2021 23:15
import * as pickerPropTypes from './pickerPropTypes';
import pickerStyles from './pickerStyles';

const Picker = props => (
Copy link
Member

Choose a reason for hiding this comment

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

BasePicker

Comment on lines 29 to 31
Picker.propTypes = pickerPropTypes.propTypes;
Picker.defaultProps = pickerPropTypes.defaultProps;
Picker.displayName = 'Picker';
Copy link
Member

Choose a reason for hiding this comment

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

BasePicker

Picker.defaultProps = pickerPropTypes.defaultProps;
Picker.displayName = 'Picker';

export default Picker;
Copy link
Member

Choose a reason for hiding this comment

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

same

Comment on lines 5 to 6
import * as pickerPropTypes from './pickerPropTypes';
import pickerStyles from './pickerStyles';
Copy link
Member

Choose a reason for hiding this comment

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

Let's prefix these files with base if they are only used for basePicker.

@mateusbra
Copy link
Contributor Author

@parasharrajat should we do the same to all the index.js inside the pickerstyles folder?
index.js,
index.ios.js,
index.android.js

@parasharrajat
Copy link
Member

Doesn't matter much. I leave this up to you.

@mateusbra
Copy link
Contributor Author

Ok, so I think thats it, just need your approval 😄

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 just like the other PR. 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!

@mateusbra mateusbra dismissed stale reviews from puneetlath and parasharrajat via 9414679 January 4, 2022 01:52
@mateusbra
Copy link
Contributor Author

@puneetlath I think we got a problem with ExpensifyText, it looks like we still have a file calling ExpensifyText instead of Text

@mateusbra
Copy link
Contributor Author

mateusbra commented Jan 4, 2022

you think its ok to deal about it in this PR? or may I open another PR dealing that? What you think we should do?

@mateusbra
Copy link
Contributor Author

mateusbra commented Jan 4, 2022

@puneetlath I think we got a problem with ExpensifyText, it looks like we still have a file calling ExpensifyText instead of Text

looks like some PR was merged using ExpensifyText instead of Text after we merged the renaeme ExpensifyText PR

@parasharrajat
Copy link
Member

parasharrajat commented Jan 4, 2022

@mateusbraCould you fix that issue as you are in charge of these changes? I don't think we mind one extra change in this PR if it is all about renaming.

The fix is merged. You can pull main.

@mateusbra
Copy link
Contributor Author

thanks @parasharrajat 😄

Comment on lines 152 to 153
versionCode 1001012419
versionName "1.1.24-19"
versionCode 1001012422
versionName "1.1.24-22"
Copy link
Member

Choose a reason for hiding this comment

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

How are these changes related to this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure. But when we merge main, do the new changes show in PR changes?

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 think they shoudn't show, I'm not understanding why its showing, we are currently using version 1.1.24-22 it shouldn't show as a 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'll try to revert the last commit changes

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.

Seems like the test will pass. SO LGTM. Nice work.

cc: @puneetlath
🎀 👀 🎀 C+ reviewed

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 when I try to compile it it's failing because it seems there is still a reference to ExpensifyText in src/pages/home/report/ReportActionItemMessage.js even though there is no longer an ExpensifyText component.

@puneetlath puneetlath self-requested a review January 4, 2022 18:28
@puneetlath
Copy link
Contributor

Actually, looks like that is addressed here: https://expensify.slack.com/archives/C01GTK53T8Q/p1641299616468900

@puneetlath puneetlath merged commit 667570c into Expensify:main Jan 4, 2022
@OSBotify
Copy link
Contributor

OSBotify commented Jan 4, 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