mikaelacaron / Basic-Car-Maintenance

A basic app to track your car's maintenance. Open source for Hacktoberfest 2023. Beginners are welcome!
https://mikaelacaron.github.io/Basic-Car-Maintenance/documentation/basic_car_maintenance/
Apache License 2.0
214 stars 123 forks source link

fix: Prevent Users From Deleting All Vehicles #276

Closed windrunner21 closed 9 months ago

windrunner21 commented 9 months ago

What it Does

How I Tested

Notes

Screenshot

https://github.com/mikaelacaron/Basic-Car-Maintenance/assets/18750749/005af7ad-499a-46aa-a531-1ecf0dc78aaf

n recording:

windrunner21 commented 9 months ago

@mikaelacaron I have also added some localization options for Turkish and Russian languages for the Alert I have created. I know English, Azerbaijani, Russian and Turkish languages. Also have limited knowledge in Japanese and French. If any help is needed in localization part, feel free to ask as well!

windrunner21 commented 9 months ago

@mikaelacaron hey, just checking up on you 😁 anything you need?

mikaelacaron commented 9 months ago

@windrunner21 Thanks! Sorry I haven't had a chance to look at this yet! I'll get to it this weekend

windrunner21 commented 9 months ago

Sure, looking forward to your comments! Good luck in the meantime!

Jakayus commented 9 months ago

@windrunner21 @mikaelacaron This is a suggestion, but I am wondering if we can improve UX with the alert message text as the user might question why we can't delete the last vehicle. I used ChatGPT to quickly brainstorm some message versions and these seemed like the best.

"The last vehicle can't be deleted. Please ensure at least one vehicle remains in your account." "You must have at least one vehicle in your account. Please add a new vehicle before deleting this one." "Oops! Looks like this is your only vehicle. To keep using our services, you need at least one vehicle in your account."

Those are just suggestions/ideas! I also don't want to add a bunch of last minute work to the PR, especially considering localization efforts needed.

windrunner21 commented 9 months ago

@Jakayus hey, I agree with you as I was under the same impression while writing this feature. However, the reason I didn't decide to mention it to @mikaelacaron is that I think this feature is temporary, and this restriction may be removed in the future. If not, then it's not a problem for me, let's decide on the improved prompt and I'll add it!

Good catch!

Jakayus commented 9 months ago

@windrunner21 Thank you for the background!

I would say let's go with The last vehicle can't be deleted. Please add a new vehicle before removing this one. This already includes some of what you have previously, so the localization is already halfway there. It's more action oriented than reason oriented, not sure what we prefer for UX.

I think too if we want to use a fancier/friendlier prompt, perhaps @mikaelacaron can weigh in on specific wording. The above is sort of the quickest fix.

mikaelacaron commented 9 months ago

However, the reason I didn't decide to mention it to @mikaelacaron is that I think this feature is temporary, and this restriction may be removed in the future.

All good! I'm totally fine with you (and anyone) bringing up any suggestions for how I've written any feature!

This issue, the reason for it right now is because if you try to add a maintenance event or odometer reading, without having a vehicle, the row only says "Select a vehicle" without anything else. And because the vehicle doesn't exist, the user would need to figure out that they have to go back to settings to add the vehicle.

Adding an onboarding flow #40 prevents this from happening (for this MVP), but the user can still delete vehicles, so the overall issue still exists. #202 prevents the user from deleting all vehicles so this isn't a problem

Later on after the 1st version is shipped I'm thinking of a longer term solution to this issue

mikaelacaron commented 9 months ago

I have also added some localization options for Turkish and Russian languages for the Alert I have created.

Oh also @windrunner21 when adding new features, don't worry about not being able to translate everything into all the different languages this app supports (if you do know a language the app supports, like Russian) you can go ahead and add the translation for the new string

If you would like to add a new language that isn't supported yet, you can comment on #7 what language you'd like to add, and then open a PR with only those changes

windrunner21 commented 9 months ago

This issue, the reason for it right now is because if you try to add a maintenance event or odometer reading, without having a vehicle, the row only says "Select a vehicle" without anything else. And because the vehicle doesn't exist, the user would need to figure out that they have to go back to settings to add the vehicle.

@mikaelacaron I think we can display "Add vehicle" button instead of "Select a vehicle" row in this user story, in order to make it clear for the end user to add their vehicle first. If you think that this solutions makes sense, I can work on it if you'd like.

mikaelacaron commented 9 months ago

@mikaelacaron I think we can display "Add vehicle" button instead of "Select a vehicle" row in this user story, in order to make it clear for the end user to add their vehicle first. If you think that this solutions makes sense, I can work on it if you'd like.

This doesn't make sense right now because the user will try to tap on it, and still nothing happens

If you want to see this, in your simulator do "erase all settings" and then run the app, and you'll see