datetimepicker icon indicating copy to clipboard operation
datetimepicker copied to clipboard

Update RNDateTimePickerShadowView.m

Open bmichotte opened this issue 4 months ago • 8 comments

Summary

This PR fixes #866 as suggested by @sbeigel in https://github.com/react-native-datetimepicker/datetimepicker/issues/866#issuecomment-1980375427

Test Plan

Build a RN app with xcode 15.3, you'll receive the error shown in #886

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Build in xcode 15.3

Compatibility

OS Implemented
iOS
Android

Checklist

  • [x] I have tested this on a device and a simulator
  • [ ] I added the documentation in README.md
  • [ ] I updated the typed files (TS and Flow)
  • [ ] I added a sample use of the API in the example project (example/App.js)
  • [ ] I have added automated tests, either in JS or e2e tests, as applicable

bmichotte avatar Mar 06 '24 14:03 bmichotte

@bmichotte I tried mocking this fix in my local to build an RN App in iOS 17.0.1. But I got these errors. Are these related?

Screenshot 2024-03-06 at 2 52 11 PM

rahie-works avatar Mar 06 '24 19:03 rahie-works

@bmichotte I tried mocking this fix in my local to build an RN App in iOS 17.0.1. But I got these errors. Are these related?

Screenshot 2024-03-06 at 2 52 11 PM

Please disregard: This was fixed from here https://github.com/itinance/react-native-sha256/commit/77af2681181cd2e31e46d57909ec302f14ee24fb

rahie-works avatar Mar 06 '24 20:03 rahie-works

Thx! This patch worked for me.

We'll apply it locally for now with npx patch-package @react-native-community/datetimepicker

jzaefferer avatar Mar 07 '24 10:03 jzaefferer

Tks u!! This worked for me too! Please merge it.

f0ntana avatar Mar 08 '24 01:03 f0ntana

Is there any reason why this is not merged yet?

Robert27 avatar Mar 13 '24 13:03 Robert27

Failed an automatic test.

I think it should be manually analyzed/approved and merged asap

fellipe-ribeiro avatar Mar 13 '24 13:03 fellipe-ribeiro

Can we merge this asap?

fnazarios avatar Mar 14 '24 11:03 fnazarios

Is this repo still maintained?

Robert27 avatar Mar 14 '24 12:03 Robert27

@luancurti @vonovak Hi guys, Could we have some attention here? tks.

f0ntana avatar Mar 18 '24 10:03 f0ntana

Hello, sorry for taking the time. The automated tests are failing, as you can see. We run automated tests in order to ensure the quality of what we're shipping. What needs to happen is that someone needs to look into the problem; probably run the tests locally and probably update some dependencies like Detox / React Native / ... and maybe update the pipeline so that it runs on latest Xcode.

Do we have a volunteer here? 🙂 Alternatively, I'm happy to look into it but I'd really like to see some companies that use this module to step up and contribute some funds to the maintenance. Right now, the budget is $5 per month for a package that currently has 461 392 weekly downloads(!). I explained my stance in #313. Thank you and sorry for being that guy.

vonovak avatar Mar 18 '24 10:03 vonovak

Hi @vonovak, Firstly, thanks a lot for your contribution to this library. I've noticed the CI for this PR is failing for the same reason the master branch's CI fails, indicating the issue isn't related to this PR specifically. Given the changes made here, particularly the fix to the type hint of the first argument, it seems important to merge this to address the significant bug affecting the library.

ottaviano avatar Mar 18 '24 11:03 ottaviano

hi @ottaviano, thanks for your comment. I didn't mean to imply that the CI failure is related to this change. If you look at the master branch history, you'll see it started failing for a reason that appears to be outside of this repository. In a pipeline that ran on the 15th of February, the e2e testing step passed okay (the publish step failed, but that's a different problem related to expired npm auth token, and I'm working on that). Something has changed in the mean time.

It doesn't happen often that stuff which worked previously would stop working, but it happened here. And, someone needs to do the work of fixing it (or disabling the pipeline checks, so that they are effectively ignored. That, however, would lead us down a slippery slope. Does anyone think disabling pipeline checks is a good idea?)

Even if I force-merge this PR, the change will not be released because the automated release depends on all pipeline steps completing successfully.

There is a known workaround using patch-package in apps consuming the package. The way I see it, people should use patch-package (or install from this branch) until we have all checks green. I'm sorry 🤷‍♂️ .

vonovak avatar Mar 18 '24 11:03 vonovak

Hello @vonovak, a quick search guided me to https://github.com/wix/Detox/issues/4148 which is the same error.

The error on the android is related to a test killed after 1 hour since it took over than 40 minutes to start the emulator.

Try out a larger resource class or running tests in parallel to speed up job execution. Upgrade your pricing plan to take advantage of longer max job runtimes. context deadline exceeded

As described in the Detox issue, updating Detox fix the issue, and therefore the iOS CI passes. I guess you can now merge this PR and release it :)

bmichotte avatar Mar 18 '24 14:03 bmichotte

thank you for taking the initiative! 🙂

vonovak avatar Mar 18 '24 14:03 vonovak

yeah! thanks to you two 🤝

ottaviano avatar Mar 18 '24 14:03 ottaviano

:tada: This issue has been resolved in version 7.6.3 :tada:

If this package helps you, consider sponsoring us! :rocket:

vonovak avatar Mar 18 '24 19:03 vonovak