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

Fix build in Xcode 16 #1698

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Fix build in Xcode 16 #1698

wants to merge 13 commits into from

Conversation

joshuatbrown
Copy link
Contributor

@joshuatbrown joshuatbrown commented Nov 27, 2024

Issues covered

#1570
https://github.com/verse-pbc/issues/issues/46

Description

Fixes the issues in Xcode 16:

  • lots of warnings
  • tab bar is broken on iPad/Mac (tested in iOS simulator on iPad, not tested on macOS Sequoia)
  • SwiftUI previews

How to test

  1. run the app on iPad or Mac
  2. make sure the tab bar looks good
  3. try the previews
  4. make sure there are no warnings

@mplorentz
Copy link
Member

👀

Copy link
Member

@mplorentz mplorentz left a comment

Choose a reason for hiding this comment

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

The code changes here all work great for me. Unfortunately macOS sequoia is dumb and the tab bar fix that works everywhere else does not work here.

Screenshot 2024-12-03 at 6 56 39 AM

I did some investigation and found a workaround, but it means we'd need to refactor AppView to use UITabBarController instead of TabView 😞 .

I see a few options here.

  1. Do the refactor. It probably won't take that long, right?
  2. Just leave it broken on macOS
  3. Continue developing in Xcode 16, but have Github build our app with Xcode 15, punting on the issue.
  4. Implement the custom tab bar that Sebastian designed.

I feel like the first makes the most sense to me. It would be fun to do the 4th too. Let me know what you think.

@@ -4,8 +4,10 @@ import SwiftSoup
/// Parses the Open Graph metadata from an HTML document using SwiftSoup.
struct SoupOpenGraphParser: OpenGraphParser {
func metadata(html: Data) -> OpenGraphMetadata? {
let htmlString = String(decoding: html, as: UTF8.self)
guard let document = try? SwiftSoup.parse(htmlString) else { return nil }
guard
Copy link
Member

Choose a reason for hiding this comment

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

looks like the formatting got messed up here. I'm surprised it didn't trigger swiftlint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that was intentional. I had to add the let htmlString to the guard and then I wanted the let statements to line up. SwiftLint was good with it and Xcode formats this way so I thought it'd be OK. Is there another format you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the condition to start on the same line as the control flow statement. I don't think I've ever seen them split apart like this before, certainly not in the Nos codebase, so it was harder for me to read. It's nice that the lets line up but in general I think I prefer the consistent indentation over visual symmetry.

However it's not really a hill I want to die on. I could get used to this.

@joshuatbrown
Copy link
Contributor Author

@mplorentz Made it a UITabBarController and everything is working! I tested in simulators as well as on macOS Sequoia and macOS Sonoma, and I think we're all set. But please test it yourself to be sure. I'm not 100% satisfied with this since I kicked a can or two down the road (see the @preconcurrency attributes) but I think those are probably OK for now.

@mplorentz
Copy link
Member

👀

Copy link
Member

@mplorentz mplorentz left a comment

Choose a reason for hiding this comment

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

This looks good. The pixels changed quite a bit on macOS, I wish there was more padding at the bottom. Here's a screenshot comparing Nos Dev (this branch), Nos Staging, and the Figma design:
Screenshot 2024-12-09 at 5 25 28 PM

Also, the profile icon got weirdly brighter :(

The tab bars on the iPhone look the same, except for the profile icon which is noticeably brighter than the others. I'm not sure if it's worth spending time on, but it's pretty obvious to me.

@mplorentz
Copy link
Member

Actually maybe the icons other than the profile icon got dimmer? 🤔

@@ -52,5 +52,8 @@
"info" : {
"author" : "xcode",
"version" : 1
},
"properties" : {
"template-rendering-intent" : "original"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mplorentz this fixes the issue with the profile icon. I'm surprised we didn't see the issue on previous builds but I guess maybe something changed in Xcode 16 or 16.1.

@joshuatbrown
Copy link
Contributor Author

joshuatbrown commented Dec 10, 2024

After the update, here's a screenshot from macOS Sequoia 15.1.1:

Screenshot 2024-12-10 at 10 48 29 AM

I think the tab bar height was broken because of the size class hack (horizontalSizeClass = .compact). The solution -- which adds bottom padding -- isn't perfect but I'm not sure of a better way. It at least feels like it can breathe now.

macOS Sonoma and earlier should now be back to their original behavior; the tab bar should look good there. I can take a screenshot and post it in a bit.

@joshuatbrown
Copy link
Contributor Author

joshuatbrown commented Dec 10, 2024

And here it is on macOS Sonoma:

Screenshot 2024-12-10 at 11 13 42 AM

And the iOS simulators:

iOS 18 iOS 17
Simulator Screenshot - iPhone 15 Pro - 2024-12-10 at 11 19 05 Simulator Screenshot - iPhone 15 Pro - 2024-12-10 at 11 17 27

@mplorentz
Copy link
Member

This looks great on the Mac now, but the iPad has reverted to the new tab bar style :/

simulator_screenshot_56AAB80A-2F40-4C03-ABC0-2E26143B9305

@joshuatbrown
Copy link
Contributor Author

Oops -- ok, I'll fix iPad and try to be more careful about testing everything next time. Missed testing on iPad. 🤦

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.

2 participants