criticalmaps / criticalmaps-ios

Critical Maps iOS App 🚲✊
http://www.criticalmaps.net
MIT License
283 stars 40 forks source link

Rebuild of the app #429

Closed mltbnz closed 2 years ago

mltbnz commented 2 years ago

πŸ“ Docs

πŸ“² What

This is essentially a rebuild of the App built with The Composable Architecture and SwiftUI. The project consists of a Swift package which bundles the individual modules for the app. Building the app in modules allows to work on features without building the entire application, which improves compile times and SwiftUI preview stability. Also every feature is its own target which makes it also possible to build mini-apps to run in the simulator for preview.

Missing The Friends feature is currently missing.

πŸ€” Why

Maintaining the codebase became a bit hard and since I decided I wanted to keep maintaining the project I wanted a codebase which is both maintainable and modern to work on. There are a lot of benefits as well:

πŸ‘€ See

Screenshots, external resources?

Screenshot 2021-12-30 at 11 41 51 Screenshot 2021-12-30 at 11 38 35 Screenshot 2021-12-30 at 11 38 30 Screenshot 2021-12-30 at 11 37 59 Screenshot 2021-12-30 at 11 38 07 Screenshot 2021-12-30 at 11 38 26 Screenshot 2021-12-30 at 11 38 24 Screenshot 2021-12-30 at 11 38 18 Screenshot 2021-12-30 at 11 38 15

♿️ Accessibility

πŸ”„ Review & Merge

This PR will replace all files on main and therefore needs to be merged forcefully. I preserved the current state of the app on the branch release/3.9.2 to keep around. Review of the code is optional but I would like you to check out the branch, launch the so the see if anything pops out that needs fixing. Before a potential release I would also like to do a new version for Testflight and maybe a tweet so we might get some people to test this rebuild.

fbernutz commented 2 years ago

Hey @mltbnz! Amazing job with the rewrite! πŸ‘

I just had a look at it (finally) and here's some small feedback:

I think all of these issues can be fixed in a later version as well. The rest looks really great already! ✨ Thumbs up from me!

Some screenshots: 1 2 3 4
Screen Shot 2022-01-02 at 09 38 20 Simulator Screen Shot - iPhone 8 - 2022-01-02 at 09 33 14 Simulator Screen Shot - iPhone 8 - 2022-01-02 at 09 37 19 Simulator Screen Shot - iPhone 8 - 2022-01-02 at 09 33 25
mltbnz commented 2 years ago

Thanks. The points should be fixed easily so I'll address them in this PR.

The failing test is currently a bit of a mystery. Don't get why these are different on the CI run. I recorded the Snapshots with an iPhone 12 btw. I'll update the PR template.

The toggle action hopefully is just an accessibilityAction modifier πŸ˜„

fbernutz commented 2 years ago

Mhmmm that's weird, the test even fails on iPhone 12 for me on my machine.

mltbnz commented 2 years ago

Ok. Then I should double check it again if what I have checked in is even valid πŸ™ˆ

mltbnz commented 2 years ago

seems like my update snapshot fixed it πŸŽ‰ I addressed the issues. Thanks for taking the time to check it out.

Do you have an idea what would be the best way to merge it back to main?

fbernutz commented 2 years ago

@mltbnz Amazing!

Not sure if this is the easiest way but I would go with this:

I think the most important thing is to not force push the main branch, so others don't have to fix their local branches.