search

cadets-ca / ets

MIT License
5 stars 3 forks source link

Various Changes #58

Closed kirvanp closed 4 years ago

kirvanp commented 4 years ago

1) Updates the target to iOS 13.4 and corrects all of the associated warnings except those related to the rarely used bluetooth features. I will do these later. Updating the target will allow us to use newer APIs that will make the iCloud syncing more reliable and it will make the download size of the app smaller. Removing the code deprecated in iOS 11-13 while still supporting iOS 10 would be an alternative but would require more code 2) Fixes hundreds of dark mode related problems, mostly by changing text in various places from black to default, where the default color automatically switches between black and white 3) Fixed the missing Done buttons on the iPhone

huguesfcadets commented 4 years ago

Pull request rejected for the following reasons:

kirvanp commented 4 years ago

Hi folks,

A few discussion points.

From my point of view, the most critical work to do on this app is to improve the reliability of the cloud syncing both to ensure that data is accessible from all devices for report generation and more importantly to ensure all data is backed up. Currently, there are several thousand lines of code devoted to doing this in CloudKit controller. Thousands of lines can be removed by using the new NSPersistentCloudKitContainer feature introduced in iOS 13. In addition, there is a huge amount of code dedicated to finding out when the network is reachable and scheduling transmissions. This also is no longer necessary. There is a lot of potential to increase reliability here. I am willing to attempt this now, but that may change in a few weeks if my colleagues come down with COVID and I have to take on additional medical work.

Second, the app is currently a disaster in dark mode as the text shows up black on black and cannot be read. All it will take to get the app rejected again is a reviewer switching to dark mode for 5 seconds. The way I've fixed dark mode here is by using the new UIColor.labelColor which automatically switches between black and white. If we are going to support older OSes this will not work. Other options are:

  1. Include a flag in the app plist that disables dark mode
  2. Include conditional code that checks which OS and makes the text either black or labelColor depending if we are on iOS 13 or older. Unfortunately, since the text color in many places is set from the Storyboard, this would involve writing a lot more code than we have now

As a nitpick, iOS 13.4 is not in beta. It is in Golden Master and is currently the OS that Apple's reviewers are testing on, so it is important that the app runs well on it. It will be released to the public at or before 1300 tomorrow Eastern time.

The changes I suggested, while they may seem extensive, are very basic and consist of only three things:

  1. Changing the color of text to handle dark mode. This requires iOS 13.
  2. Checking the traitCollection in viewWillAppear to deal with the done button issue. As an aside, while it is. This can be done in iOS 10, however we would need to test it against 10, 11, & 12 to ensure it doesn't introduce bugs. At the very least, it may result in the trait collection being checked twice on some of those systems
  3. Changing the row actions to UIContextualActions to remove the use of deprecated APIs and API warnings. This requires iOS 11.

Anyway, enough from me. What do you suggest?

LukeTowers commented 4 years ago

@kirvanp it might be easier for @huguesf to review the changes if you broke it down into a separate PR for each of those listed changes (assuming they don't interact with each other).

Let me know if you want any help with that

kirvanp commented 4 years ago

You could certainly break out the UIContextualAction changes (I believe there is 4) and the traitCollection fix (around 8). There's also a few changes specific to PDF generation, but it doesn't look like that code is in use now that we have the Excel.

That would leave the vast majority of the edits in the dark mode category. On the bright side, these are low risk edits- worst case scenario text isn't the right color.

huguesfcadets commented 4 years ago

Guys,

Thank you for your input.

Moving the discussion to #59 .

See you there!