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

Remove most imports of UIKit #801

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

twizmwazin
Copy link
Contributor

Checklist

  • I have read CONTRIBUTING.md
  • I have described what this PR contains
  • This PR addresses one or more open issues that were assigned to me:
    - list issue(s) here
  • If this PR alters the UI, I have attached pictures/videos

Pull Request Information

About this Pull Request

Describe what this PR contains in as much detail as possible
This PR remove almost all of the references to UIKit. Most of these were nothing more than deleting the import, the photo saving required a change. I would appreciate if someone could check I did that correctly!

Screenshots and Videos

In case this PR changes something in the UI, please include screenshots or videos of this new feature
None, any functional change is a bug

Additional Context

Any additional context you'd like to add to help us review this PR
Removing UIKit dependency and being 100% on SwiftUI would allow native Mac support someday.

@twizmwazin twizmwazin requested a review from a team as a code owner December 8, 2023 08:54
@twizmwazin twizmwazin requested review from JakeShirley and EricBAndrews and removed request for a team December 8, 2023 08:54
if success {
print("Save finished!")
} else {
print("Error saving photo: \(String(describing: error?.localizedDescription))")
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably good to call out we should maybe handle this better in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, OOS here but I've opened an issue: #802

@@ -7,10 +7,9 @@
//

import SwiftUI
import UIKit

/// A class responsible for displaying important notifications to the user
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this an enum, this makes it a value type instead of a reference type.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@EricBAndrews EricBAndrews left a comment

Choose a reason for hiding this comment

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

Looks good to me--image saving works as intended, and the unused import removal should be guaranteed safe by the compiler. I'm going to hold off on merging until a team member with more significant UI experience has had a chance to look through, though.

@EricBAndrews
Copy link
Member

Should be good to merge with a rebase + passing CI

@twizmwazin
Copy link
Contributor Author

As far as I can tell the CI is failing due to my fork not having write permission on some resource. The tests themselves appear to be passing.

@EricBAndrews
Copy link
Member

Looks like it. Build and test works fine on my machine--bypassing branch protections to merge w/o passing CI.

@EricBAndrews EricBAndrews merged commit 341c999 into mlemgroup:dev Dec 13, 2023
EricBAndrews pushed a commit that referenced this pull request Dec 18, 2023
EricBAndrews pushed a commit that referenced this pull request Dec 18, 2023
EricBAndrews pushed a commit that referenced this pull request Dec 18, 2023
EricBAndrews pushed a commit that referenced this pull request Dec 18, 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.

3 participants