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

Connect WelcomeView and WelcomeViewAddVehicle to App Flow #301

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

maartinj
Copy link
Contributor

@maartinj maartinj commented Aug 4, 2024

What it Does

  • Closes IMPROVE - Connect WelcomeView and WelcomeViewAddVehicle to App Flow #285
  • When app launch for the first time, will be presented OnboardingView
  • OnBoardingView contains 2 screens: WelcomeView and WelcomeViewAddVehicle
  • WelcomeView present the highlighted features of the app
  • WelcomeViewAddVehicle allows to add basic data about first car (Vehicle Name, Make & Model)
  • This pull request contains App Flow where User using the app for the first time, will see the WelcomeView, and then push WelcomeViewAddVehicle after tapping continue
  • Changed the "Welcome 🥳" button to be "Add Vehicle"
  • Once "Add Vehicle" is tapped, this Onboarding flow is dismissed, and the User, after confirming in presented alert that first vehicle data is added, should see the DashboardView. When clicked "OK" on presented alert, the app will start always from the DashboardView
  • Added temporary vehicle data to be presented in SettingsView as "Locally Saved Vehicle" to visualize correctly working Onboarding App Flow. This data can be deleted by swiping the added temporary vehicle data from right to the left. Temporary vehicle data is visible when app is restarted and app remember this data using UserDefaults functionality

How I Tested

  • Run the application in Simulator (iPhone 15 Pro, iOS 17.5), set language to 'English'
  • WelcomeView appear, and suddenly dismiss and shows alert "This is a Test Message From the Real Time Alert System" (I cannot turn this off. It looks like this alert is presented from server side)
  • Click "Dismiss" button in alert "This is a Test Message From the Real Time Alert System"
  • WelcomeView shows again
  • Click "Continue" button, WelcomeViewAddVehicle appear
  • Enter "Vehicle Name" in "Name" field, click "Add Vehicle" button
  • Alert "Vehicle Make cannot be empty! 🚗" is presented, click "OK"
  • Enter "Vehicle Make" in "Make" field, click "Add Vehicle" button
  • Alert "Vehicle Model cannot be empty! 🚗" is presented, click "OK"
  • Enter "Vehicle Model" in "Model" field, click "Add Vehicle" button
  • Alert "Congratulations! 🎉 🚙 From now on, next screen will be the main app screen" is presented, click "OK"
  • WelcomeViewAddVehicle dismiss, the DashboardView appear

Notes

  • May need further updates to include logic from SettingsView or other different task to add properly Vehicle data and delete temporary added vehicle data function

Screenshots

Screencast

onboardingFlowVideo.mp4

@mikaelacaron
Copy link
Owner

Great thank you! I may not get to this for a week or so to review @maartinj

Copy link
Owner

@mikaelacaron mikaelacaron left a comment

Choose a reason for hiding this comment

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

Thanks so much for the contribution forever ago, and I'm just now getting to review it!
I have several comments that need resolved please. If you would like you can continue this PR and make the changes I suggested, or if you'd like to pass on this issue, you can un-assign yourself and close the PR

Once you fix everything, please click the re-review button

…-appflow'

#Conflicts:
#	Basic-Car-Maintenance/Shared/Localizable.xcstrings
#	Basic-Car-Maintenance/Shared/Onboarding/Views/WelcomeViewAddVehicle.swift
#	Basic-Car-Maintenance/Shared/Settings/Views/SettingsView.swift
@maartinj
Copy link
Contributor Author

maartinj commented Oct 7, 2024

I sent corrected PR. Please check and share your thoughts.

Copy link
Owner

@mikaelacaron mikaelacaron left a comment

Choose a reason for hiding this comment

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

@maartinj something happened...
there's a bunch of changes reverting several PRs recently

you should merge dev into your branch

and click "resolve" for all the comments you addressed that you don't have questions on and have fixed, and then click re-review

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Need to integrate other changes with onboarding branch
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@maartinj
Copy link
Contributor Author

maartinj commented Oct 8, 2024

I sent corrected PR. Fingers crossed that now is fine. Please check and share your thoughts.

@mikaelacaron
Copy link
Owner

@maartinj can you check, there might be a settings somewhere about when the PR was created about allowing edits from the maintainer...?
normally I'm able to commit and push to PRs, but yours isn't letting me

If you edit the description...? it'll maybe come up...? or somewhere on the right side?

@maartinj
Copy link
Contributor Author

@mikaelacaron This box was unticked, do not know why:

pr301settings

Nevertheless, I ticked this box, so you should have access now. Please check and let me know 👍

Copy link
Owner

@mikaelacaron mikaelacaron left a comment

Choose a reason for hiding this comment

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

I've made several comments about how data is passed and things to fix. I've resolved most of them, and left the comments for you, for next time!

don't change anything, I'm still deciding how I want some things to function

}
}

var message: String? {
Copy link
Owner

Choose a reason for hiding this comment

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

this and the title should be a LocalizedStringResource so that they can be translated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You fixed this also, right? I see in commit 'PR 301 Suggestions, moving around functionality', in line 171, that is added with localizedStringResource?

@@ -17,10 +18,10 @@ struct WelcomeView: View {
HStack(spacing: 5) {
Text("Welcome to")
Text("Basic")
.foregroundStyle(Color("basicGreen"))
Copy link
Owner

Choose a reason for hiding this comment

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

note we don't need to use strings anymore because assets automatically generate type safe names like .basicGreen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You fixed this also, right? I see in commit 'fix colors, capitalization and comments', you changed all .basicGreen?

@mikaelacaron
Copy link
Owner

I'm flying today and will look back at this, don't change anything! thank you! it's almost ready to merge
@maartinj thanks for the work!

@maartinj
Copy link
Contributor Author

I'm flying today and will look back at this, don't change anything! thank you! it's almost ready to merge @maartinj thanks for the work!

Sure @mikaelacaron, I am waiting for your other comments 😊👍

@mikaelacaron mikaelacaron added the hacktoberfest-accepted Accepted PR for Hacktoberfest label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted PR for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IMPROVE - Connect WelcomeView and WelcomeViewAddVehicle to App Flow
2 participants