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

Feature/native dark mode support #16

Conversation

NinjaSnail42
Copy link
Contributor

@NinjaSnail42 NinjaSnail42 commented Mar 11, 2020

#4 Native Dark Mode support for the app.

Notes

  • To change the text color of the TextViews with attributed text, I had to do it programatically. Since this is the case, I converted the Objective-C code for those three view controllers (which I am currently working on on a different branch anyway.)
  • I did not change the way that the blue color in the app appears between light and dark mode. I don't think that it looks bad in dark mode, but if we are interested in changing the color between light and dark mode, there are a couple things to know. You can do this by adding color sets, but this is not supported the on older versions of iOS that the app supports. It could also done programmatically, but will be extra work and would be best to do after the entire app is converted from Objective-C to Swift.

Testing

  • I tested on iOS Simulators with iOS 10.3 (iPhone 5s) and iOS 13 (iPhone 11 Pro Max).

Had to convert GuideCopingViewController to Swift and update the color of the text programatically in the Coping ViewController.
• Converted some files to Swift to update text color programatically for attributed text.
• Changed the color of the table rows so that they are now white/dark grey instead of light grey/dark grey.
Copy link
Contributor

@shayneptorres shayneptorres left a comment

Choose a reason for hiding this comment

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

Just some small changes that I would recommend. These aren't breaking anything but would be good to keep app consistent

SafetyPlan/GuideCopingViewController.swift Outdated Show resolved Hide resolved
SafetyPlan/GuideRecoveringViewContoller.swift Outdated Show resolved Hide resolved
SafetyPlan/GuideSuicidePreventionViewController.swift Outdated Show resolved Hide resolved
SafetyPlan/GuideCopingViewController.swift Outdated Show resolved Hide resolved
SafetyPlan/GuideRecoveringViewContoller.swift Outdated Show resolved Hide resolved
SafetyPlan/GuideCopingViewController.swift Show resolved Hide resolved
SafetyPlan/GuideRecoveringViewContoller.swift Show resolved Hide resolved
SafetyPlan/GuideSuicidePreventionViewController.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@shayneptorres shayneptorres left a comment

Choose a reason for hiding this comment

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

Looks great, just make sure you fix the merge conflicts. I also agree we should add a .gitignore

@eddielement
Copy link
Contributor

eddielement commented Mar 11, 2020

I just checked over it now and it looks great to me too just with a few merge conflicts to fix. Agreed on the gitignore (although I've tried and failed in the past at ignoring that- maybe there are lots of different DS_Stores?). Thanks for your input too @shayneptorres !

@NinjaSnail42
Copy link
Contributor Author

Yeah, apologies for the merge conflict on the storyboard file. I was a dev manager on an iOS project last year for a class at University, and it is quite a pain to fix merge conflicts in the storyboard files.

@NinjaSnail42
Copy link
Contributor Author

I fixed the merge conflicts. Had to change repositories from my fork to the actual repo. Should be good for an admin to merge now.

Match the Crisis page match the Guide page
@eddielement
Copy link
Contributor

Whew thank you for that @NinjaSnail42 !! I tried to resolve the conflicts for 30 minutes earlier, it was quite hellish, and I ended up discarding all my changes and was going to try again tonight. I made a few tweaks and I'm merging the changes in now! :)

@eddielement eddielement merged commit d6a4713 into suicidesafetyplan:master Mar 11, 2020
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.

None yet

3 participants