-
Notifications
You must be signed in to change notification settings - Fork 4
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
Generosity challenge - day 2 - assignment - select family values (kids-923) #711
Generosity challenge - day 2 - assignment - select family values (kids-923) #711
Conversation
@@ -11,6 +11,10 @@ enum Pages { | |||
path: '/generosity-challenge', | |||
name: 'GENEROSITY-CHALLENGE', | |||
), | |||
generositySelectValues( | |||
path: 'select-values', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add "generosity-..." for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would reather remove generosity from the title to make it selectValues(path: 'select-values',) since it is on the generosity sub-route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense bc of sub route, I didn't think about it)
required this.area, | ||
}); | ||
|
||
factory FamilyValue.fromJson(Map<String, dynamic> json) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps fromMap/toMap would be more consistent?
mainAxisAlignment: MainAxisAlignment.spaceBetween, | ||
children: [ | ||
Text( | ||
'Select 3 values', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we use FamilyValuesState.maxSelectedValues here?
|
||
mixin FamilyValuesRepository { | ||
Future<bool> rememberValues({ | ||
required List<Map<String, dynamic>> body, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we use map here instead of List?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its of list of maps because its a list of family values, i could wrap the list in a map to be something like:
{"values" : [
{ ...
},
{ ...
},
]
}
But i am not sure what would be the utility of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Its of list of maps because its a list of family values' - yes, and that's ok. I think repository is a good place for data translation from model to map and to json string and backward instead of doing it in the cubit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this part:
final selectedValuesJson = state.selectedValues.map((e) => e.toMap()).toList();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh! Yes totally, thank you for the direction 🙏
@@ -12,21 +12,18 @@ class Task extends Equatable { | |||
required this.description, | |||
required this.onTap, | |||
required this.buttonText, | |||
required this.feedbackImage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to keep feedback per day logic since it's already implemented and can be useful for upcoming challenges, even if we do not use it for the current one (I assume bc of simplification).
'day': challenge.state.detailedDayIndex + 1, | ||
}, | ||
); | ||
await challenge.completeActiveDay(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at this point I wanna ask why don't we use GenerosityChallengeCubit for it? Isn't DailyAssignmentCubit look redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to use smaller, more 'focused', but youu are right, one more GenerosityChallengeStatus would work as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
imagePath: | ||
'https://givtstoragedebug.blob.core.windows.net/public/cdn/tag-logos/aid.svg', | ||
colorCombo: ColorCombo.highlight, | ||
interestKeys: ['AFTERADISASTER'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's put these values in enums for both interestKeys and area
…ect-family-values # Conflicts: # lib/app/injection/injection.dart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job Daniela!
Description
Overview of changes: