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

Rebuild of the app #429

Merged
merged 116 commits into from
Jan 4, 2022
Merged

Rebuild of the app #429

merged 116 commits into from
Jan 4, 2022

Conversation

mltbnz
Copy link
Member

@mltbnz mltbnz commented Dec 30, 2021

📝 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:

  • uniform feature architecture
  • high testability
  • modularisation
  • enables easy porting to other platforms
  • managing dependencies and side effects in an understandable way

👀 See

Screenshots, external resources?

  • Added snapshot tests

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

  • Tap targets use minimum of 44x44 pts dimensions
  • Works with VoiceOver
  • Supports Dynamic Type

🔄 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.

mltbnz added 30 commits June 12, 2021 12:19
+ add tests
+ add coordinate obfuscation
* pull decode upstream
* info bar wip

* add cm infobar icon

* setup infobar window in scenedelegate

* permanantly get user locations

* fix apirequest and nextRideRequest

* update styleguide

* add infobar tests

* fix next ride

* update info bar and add swipe

* add infobannercontroller mock

* update infobar tests

* fix container viewController

* update adding child controller structure

* update swipe up trigger value

* inject date as func

* use styleguide values

* add infobar testtarget to testplane

* add trackingMode delegate and set binding value

* remove dead code

* use styleguide grid

* update mapview edges

* fix tests

* fix mktrackingmode reset

* bump deployment to ios 14

* move resources to styleguide

* add cm annotation to map

* update Assets to L10n
when tapping on the infobar it swipes out of the screen and the nextRide gets focused on the map
create snapshot tests for existing views
+ also adds a custom navigationbar
+ update backgroundSecondary color
* update test

* fix colors

to test on none beta versions

* test

* update ci yaml 1

* update ci yaml 2

* update ci yaml 3

* update ci yaml

* update fastlane

* upload artifacts when failure

* upload artifacts when failure

* update snapshots

* set xcode setup

* disable snapshot tests for now
* add GuideFeature to Package

* add images for guide feature

* add GuideFeature components

* setup navigationView to navigate to GuideView

* add dismissable modifier

* remove dismissable from detailview

* set toolbaritem placement
* add settings target

* add settingsview

* update settings views

* add support section colors

* add support section icons

* remove tca import to make it build

* open settings from appnavigationview

* update a11y

* add infosection

* set version and build number

* add SettingFeatureTests

* add settingsFeatureCore and link with views

all openURL commands open a url

* update app assets

* add fileClient

* add appIcon select

WIP

* add helpers

* update helpers package

* update tests

* remove infobarcontroller in favor of @main app

* update helper

* update packages

* temp infobar fix

* tests

* save selected theme to userSettings and use light textcolor

* fix in favour of CI Version

* fix appIcon settings

+ add appearance tests

* fix app feature tests

+ add userSettingsLoaded receiver

* add custom dump package

* add event settings view

* appearance updates

* Update main.yml (#420)

* Update Gemfile.lock

* Update main.yml

update Xcode version

* Update main.yml

* Update main.yml

* update event settings enable copy

* update settings

+ add event settings

* update settings views

* add event settings tests

* update toggle color

* update settings

+ remove openurl type from settingsnavigationlink
+ fix supports views alignment

* minor ui tweaks

* refactor appearance core

* add settings snapshot tests

* update ui

* update testplan

* refactor rideeventsettings

+ add core and connect rideeventreducer to settingsreducer

* update snapshot tests

+ use only one device for tests
+ update sloppy precision to 0.99
* only set unread messages when chat view is not presented
* docs 📝
* test ✅
@mltbnz mltbnz requested a review from a team as a code owner December 30, 2021 11:08
@mltbnz mltbnz changed the title Develop Rebuild of the app Dec 30, 2021
@fbernutz
Copy link
Member

fbernutz commented Jan 2, 2022

Hey @mltbnz!
Amazing job with the rewrite! 👏

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

  • The failing test AppNavigationViewSnapshotTests.test_appNavigationView_WithBadge() in the CI fails locally for me as well when I run with Xcode 13.2.1 and iPhone 8 Plus with latest iOS, maybe it's because of the precision of 95%?
  • The switch for the observation mode in settings can't be activated with voice over 🤔 (1)
  • Maybe we can switch from HStack to VStack for the TweetView when the user is using an accessibilitySize with isAccessibilityCategory? (2)
  • All elements of a tweet are accessible on their own, so navigating with voice over is a bit hard on the list. Maybe we can group the elements similar to the old/current version? (3)
  • The GuideDetailView needs to have a ScrollView for larger font sizes, otherwise text will be truncated. (4)

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
Copy link
Member Author

mltbnz commented Jan 2, 2022

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
Copy link
Member

fbernutz commented Jan 2, 2022

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

@mltbnz
Copy link
Member Author

mltbnz commented Jan 2, 2022

Ok. Then I should double check it again if what I have checked in is even valid 🙈

@mltbnz
Copy link
Member Author

mltbnz commented Jan 2, 2022

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
Copy link
Member

fbernutz commented Jan 4, 2022

@mltbnz Amazing!

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

  • on develop branch: merge in the main branch with "ours" merge strategy, git merge -s ours main for solving the conflicts
  • from Github UI or locally on main branch: do a merge commit or squashed commit to merge develop into main

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

@mltbnz mltbnz merged commit f339e39 into main Jan 4, 2022
@mltbnz mltbnz deleted the develop branch January 4, 2022 10:59
@mltbnz mltbnz restored the develop branch January 4, 2022 11:01
@mltbnz mltbnz mentioned this pull request Jan 4, 2022
18 tasks
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

2 participants