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

Allow users to set Latest Completed ToDos for all cars in new user setup #261

Merged
merged 8 commits into from
Mar 3, 2020

Conversation

baylessj
Copy link
Member

@baylessj baylessj commented Mar 2, 2020

Closes #194.

@baylessj baylessj requested a review from DavBfr March 2, 2020 23:48
@baylessj baylessj removed the request for review from DavBfr March 2, 2020 23:52
@DavBfr
Copy link
Collaborator

DavBfr commented Mar 2, 2020

What is the goal of this? -- nevermind

I think the wizard few screens will eventually become more complicated than that. Maybe it's easier to configure the cars one by one.

@baylessj
Copy link
Member Author

baylessj commented Mar 2, 2020

When my parents were first setting up their accounts, they added two cars in the first of the new user setup screens. The next setup screen asks for the last time that you changed the car's oil and tires, but it didn't specify which of the two cars that it was for, and only offered 1 car's worth of input fields. This allows the user to set the latest oil change and tire change mileages for all of the car's that they added in the first new user setup screen.

I removed you from the review for now because I realized that I forgot to check if the tests were working. Turns out, unsurprisingly, they aren't working. I'm going to grab dinner now but I'll get the tests passing later this evening and add you back to the review.

@baylessj baylessj requested a review from DavBfr March 3, 2020 01:06
@codecov
Copy link

codecov bot commented Mar 3, 2020

Codecov Report

Merging #261 into develop will increase coverage by 0.07%.
The diff coverage is 86.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #261      +/-   ##
===========================================
+ Coverage    88.58%   88.65%   +0.07%     
===========================================
  Files          130      130              
  Lines         5044     5085      +41     
===========================================
+ Hits          4468     4508      +40     
- Misses         576      577       +1
Impacted Files Coverage Δ
.../welcome/views/new_user_setup/latestcompleted.dart 90.69% <86.66%> (+3.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a168f9...9a15e63. Read the comment docs.

@baylessj
Copy link
Member Author

baylessj commented Mar 3, 2020

Added you back to review the changes, unit tests are back to working now.

I agree that the onboarding will likely get more complicated in the future, and that it will make sense to set up cars one by one. That will be a pretty big reworking and would tie well into our idea of capping free tier users at one car, so maybe that could be a task for sometime soon? I can start putting together some Figma sketches or something to brainstorm the screenflow for that when the time comes.

@DavBfr
Copy link
Collaborator

DavBfr commented Mar 3, 2020

With the next PR, we'll have to add units validation first: you don't want to manually convert your mileage to kilometers if you're in the US ;)

class _TodoFieldsState extends State<_TodoFields> {
_TodoFieldsState(this.c, this.formKey, this.showName);

final Car c;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need to repeat the variables from the StatefulWidget. You can get the values using widget.c

@baylessj baylessj merged commit 49f62a7 into develop Mar 3, 2020
@baylessj baylessj deleted the 194-todo-completion-setup branch March 3, 2020 01:41
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.

Allow for setting the last todo completion date for all cars in setup
2 participants