-
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 chat is integrated with the challenge itself #717
Conversation
…igation between challege pages is implemented; chat icon with red dot is implemented
lib/app/routes/app_router.dart
Outdated
final challengeCubit = state.extra! as GenerosityChallengeCubit; | ||
return BlocProvider.value( | ||
value: challengeCubit, | ||
child: const GenerosityChallengeIntorduction(), |
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.
interoduction => introduction
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.
fixed
@@ -28,6 +48,7 @@ class GenerosityChallengeCubit extends Cubit<GenerosityChallengeState> { | |||
); | |||
|
|||
void dayDetails(int dayIndex) => emit( | |||
//maybe here i need logic |
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'd prefer if you'd elaborate the comment or if you make a ticket in Linear and then add a TODO, like:
// TODO KIDS-999 title
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.
that is not mine, but I'll delete it
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.
deleted
@@ -1,29 +1,52 @@ | |||
import 'package:equatable/equatable.dart'; | |||
import 'package:givt_app/features/children/generosity_challenge/models/enums/day_chat_status.dart'; | |||
import 'package:givt_app/features/children/generosity_challenge_chat/chat_scripts/models/chat_script_item.dart'; | |||
|
|||
class Day extends Equatable { | |||
const Day({ | |||
required this.dateCompleted, |
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.
Is there a reason why we're not using nullable for dateCompleted? I see in several places in the code that an empty string is assigned.
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.
afaik we don't have clear rules here,
however my personal prefs is to avoid nullable when possible)
final showBackButton = context | ||
.read<GenerosityChallengeCubit>() | ||
.state | ||
.availableChatDayIndex != |
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.
So I can understand this PR better, "availableChatDayIndex" refers to the "latest" available day (so not the current active index or anything else)?
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.
availableChatDayIndex actually it refers to the first non completed chat. There could be cases when active day index is not the same as active day chat index (for ex user still needs to pass through post chat of day 1, but the day 2 already available)
lib/app/routes/app_router.dart
Outdated
builder: (context, state) { | ||
final challengeCubit = state.extra! as GenerosityChallengeCubit; | ||
return BlocProvider.value( | ||
value: challengeCubit, | ||
child: const GenerosityChallengeIntorduction(), |
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.
There is no need for a BlocProvider here
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 think we do need it, since we use it inside the introduction page here:
context.goNamed( Pages.generosityChallengeChat.name, extra: context.read<GenerosityChallengeCubit>(), );
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.
Ah that's tricky, because the redirection to this page happens within _redirectFromExternalLink and we are not providing GenerosityChallengeCubit through there state.extra is null and i get a red exception screen when i get to this page currently. Funny enough when i removed the GenerosityChallengeCubit provider everything still worked as expected in the chat
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.
changed based on talk with Daniela
lib/app/routes/app_router.dart
Outdated
if (navigatingPage == Pages.generosityChallenge.path && | ||
!GenerosityChallengeHelper.isActivated) { | ||
return Pages.generosityChallengeIntroduction.path; | ||
} |
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.
because this is now a subroute it needs to be '${Pages.generosityChallenge.path}/${Pages.generosityChallengeIntroduction.path}'
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.
according to the tests, subroutes are working with just a subroute part
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.
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.
ohh, let me fix it then
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.
fixed
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 think using only the subroute works when redirecting by name, but this is using the path so it needs to be full 🤔
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.
changed based on talk with Daniela: show intro from challenge itself, instead of redirection
extra: context.read<GenerosityChallengeCubit>(), | ||
); | ||
}, | ||
icon: const Icon(FontAwesomeIcons.solidComments), |
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.
tiny thing but the icon color should be AppTheme.givtGreen40, to match the color of the text in the appbar
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.
fixed
@@ -855,10 +867,6 @@ class AppRouter { | |||
return Pages.generosityChallenge.path; |
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.
if (GenerosityChallengeHelper.isActivated || navigatingPage == Pages.generosityChallenge.path) - it's important for when users launch the app from link
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.
fixed
Description