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

chore(exoflex): Upgrade package @react-native-community/datetimepicker & react-native-modal-datetime-picker #716

Merged

Conversation

StefanusChristian
Copy link
Contributor

Changes:

  • Upgrade package @react-native-community/datetimepicker from 3.0.8 --> 6.7.5.
  • Upgrade package react-native-modal-datetime-picker from 9.1.0 --> 17.1.0.
  • Adjust usage of headerTextIOS and change it into customHeaderIOS.

The headerTextIOS changes was due to breaking changes in react-native-modal-datetime-picker: v12.0.0.
Link Issue: https://github.com/mmazzarolo/react-native-modal-datetime-picker/releases/tag/v12.0.0
Solution: mmazzarolo/react-native-modal-datetime-picker#607

@StefanusChristian StefanusChristian force-pushed the exoflex/upgrade-datetimepicker-version branch from b52d2f5 to 331c0c3 Compare September 4, 2023 08:16
@StefanusChristian
Copy link
Contributor Author

StefanusChristian commented Sep 4, 2023

It seems we have a problem in our circleci compare url orb, making the job failed without even running the test. After Josh (@ikusa) do some debugging, he told me it isn't worth pursuing this problem and better migrate this into github action.

Since this is an urgent matter (DBO Needed this changes), I'm putting the succeeded local test in here as a prove this PR is ready to be merged and address the github action migration in separate PR

test-exoflex.mp4

@ikusa
Copy link
Contributor

ikusa commented Sep 4, 2023

More background on the migrate suggestion

We met a bug here where the path-filter which is an orb / package on circleci to run job based on file change throw exit code 1. This path filter are forked of god know who hosted on yoan circleci. I tried to take a look of what CircleCi offer for dynamic job run and turns out bit more complicated as we need config splitting to support this kind of process.

Several reason why I think it's reasonable to move

  1. it's far far easier to do on GitHub Action
  2. we want to move generally as a company to only GitHub Action to avoid paying 2 providers
  3. The job definition are short and the migration would only take a day or two at worst case

Based on those reasoning I suggest we move from CircleCI

Comment on lines 25 to 30
const CustomHeaderComponent = () => (
<View style={styles.headerContainer}>
<Text style={styles.headerTextStyle}>{title}</Text>
</View>
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are creating this component on render, won't this component be recreated on every render? Though it's a simple component on a more complex component this would troublesome because I think it will keep remounting on every render. I'm not 100% sure though so maybe also need to test whether that will be the behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

I create simple sandbox here https://codesandbox.io/s/mutable-brook-z47g3z?file=/src/App.js:831-835 to test the my guess

Copy link
Contributor

@ikusa ikusa Sep 4, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in here: 3126579

Copy link
Contributor

@ikusa ikusa left a comment

Choose a reason for hiding this comment

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

Nice changes

@ikusa ikusa merged commit e06abf7 into kodefox:master Sep 5, 2023
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.

2 participants