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

A1 - Task 1: Contacts, AppIcon, and Accent Color Updated #4

Merged
merged 10 commits into from
Jan 27, 2024

Conversation

akanshyabhat
Copy link
Contributor

@akanshyabhat akanshyabhat commented Jan 16, 2024

This PR is related to Assignment 1, Task 1. The contacts have been updated to the information of the PIs. The AppIcon has been changed to an image of a white band-aid against a blue background (part of an image with no copyright) and the accent color has been chosen to match that background.

A1 - Task 1: Contacts, AppIcon, and Accent Color Updated

♻️ Current situation & Problem

The Contacts page was using generic information from the template. Similarly, the AppIcon was from the template and there was no specified accent color.

⚙️ Release Notes

  • Added images for the 3 PIs
  • Updated the contacts for the 3 PIs and added it to the ContactList
  • Added a new image for the AppIcon
  • Updated the AccentColor

📚 Documentation

No documentation was added as I only made information changes for this specific app.

✅ Testing

All of the provided tests associated with the pull request have passed.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

The contacts have been updated to the information of the PIs. The AppIcon has been changed to an image of a white band-aid against a blue background (part of an image with no copyright) and the accent color has been chosen to match that background.
One photo for a PI did not pass the reusable test. As a result, I found a different photo that should pass the test.
Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (ff4e43c) 39.98% compared to head (214ee5c) 42.69%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #4      +/-   ##
==========================================
+ Coverage   39.98%   42.69%   +2.71%     
==========================================
  Files          30       30              
  Lines         933      991      +58     
==========================================
+ Hits          373      423      +50     
- Misses        560      568       +8     
Files Coverage Δ
PICS/Contacts/Contacts.swift 78.51% <87.84%> (+9.12%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff4e43c...214ee5c. Read the comment docs.

@akanshyabhat akanshyabhat changed the title Contacts, AppIcon, and Accent Color Updated A1 - Task 1: Contacts, AppIcon, and Accent Color Updated Jan 16, 2024
Copy link

@aydinzahedi aydinzahedi left a comment

Choose a reason for hiding this comment

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

Great job with your pull request! Just a few comments:

  • I love how you documented everything using the PR template

  • Instead of copy-pasting the localized string, you can instead paste the key to the string in the string catalog like this:

description: String(localized: "YOUR BIO STRING HERE"),

  • The links in the bios don't show up correctly. You can instead create buttons like the phone and email ones on the contact screen. This bit of code could be a helpful example:

ContactOption( image: Image(systemName: "safari.fill"), // swiftlint:disable:this accessibility_label_for_image title: "Website", action: { if let url = URL(string: "https://stanford.edu") { } }

Copy link

@aydinzahedi aydinzahedi left a comment

Choose a reason for hiding this comment

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

Thanks for making your revision! Was the safari button working in your emulator? It doesn't work when I run it, and it might be because you're missing the action after the URL: UIApplication.shared.open(url). So sorry that I didn't have that included in the first comment, but you do need the action in order to allow the URL to open.

 ContactOption(
                     image: Image(systemName: "safari.fill"), // swiftlint:disable:this accessibility_label_for_image
                     title: "Website",
                     action: {
                         if let url = URL(string: "YOUR URL HERE") {
                             UIApplication.shared.open(url)
                         }
                     }

@akanshyabhat
Copy link
Contributor Author

Thanks for making your revision! Was the safari button working in your emulator? It doesn't work when I run it, and it might be because you're missing the action after the URL: UIApplication.shared.open(url). So sorry that I didn't have that included in the first comment, but you do need the action in order to allow the URL to open.

 ContactOption(
                     image: Image(systemName: "safari.fill"), // swiftlint:disable:this accessibility_label_for_image
                     title: "Website",
                     action: {
                         if let url = URL(string: "YOUR URL HERE") {
                             UIApplication.shared.open(url)
                         }
                     }

Hi! I added the action, but for some reason, it's not working. I have pushed the changes so you can take a look. Thank you!

Copy link

@aydinzahedi aydinzahedi left a comment

Choose a reason for hiding this comment

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

Hi Akanshya, were you able to get the website button to work on your end? In the simulator on my computer, the button wouldn't work. If you replace .init( with the ContactOption( struct instead, you should be good.

@akanshyabhat
Copy link
Contributor Author

Hi Aydin! I checked on the actual simulator and the button should work now!

Copy link

@aydinzahedi aydinzahedi left a comment

Choose a reason for hiding this comment

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

Great job, Akanshya!

@akanshyabhat akanshyabhat merged commit af18f0a into main Jan 27, 2024
7 checks passed
@akanshyabhat akanshyabhat deleted the akanshya-a1-task1 branch January 27, 2024 06:42
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.

3 participants