Skip to content
This repository was archived by the owner on Dec 19, 2023. It is now read-only.

refactor(capture_page): cubit usage adjustments #52

Merged
merged 2 commits into from
Feb 18, 2022
Merged

refactor(capture_page): cubit usage adjustments #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47bdb08
refactor(capture_page): cubit usage adjustments
felangel Feb 17, 2022
253b4dc
Merge branch 'capture_view' into refactor/capture_page-bloc-updates
Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions flutter_nps/lib/capture/cubit/capture_cubit.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ import 'package:flutter_nps/flutter_nps.dart';
class CaptureCubit extends Cubit<CaptureCubitState> {
CaptureCubit() : super(CaptureCubitState.initial());

bool get isScoreSelected => state.score != -1;
bool get canSubmit => state.score != -1 && state.chipIndexes.isNotEmpty;

void selectScore({required int score}) => emit(state.copyWith(score: score));

void chipToogled({required int index}) => state.chipIndexes.contains(index)
Expand Down
195 changes: 106 additions & 89 deletions flutter_nps/lib/capture/view/capture_page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,34 +60,21 @@ class CaptureView extends StatelessWidget {
const SizedBox(height: 16),
const CaptureScoreSelectorLabels(),
const SizedBox(height: 32),
AnimatedOpacity(
duration: const Duration(milliseconds: 1500),
opacity:
context.watch<CaptureCubit>().isScoreSelected ? 1.0 : 0.0,
child: Column(
children: [
const AnswerChips(),
const SizedBox(height: 32),
ElevatedButton(
key: const Key('capturePage_submit_elevatedButton'),
onPressed: context.watch<CaptureCubit>().canSubmit
? context.read<CaptureCubit>().submitResult
: null,
child: Padding(
padding: const EdgeInsets.symmetric(
horizontal: 72,
vertical: 12,
),
child: Text(
context.l10n.submit,
style: const TextStyle(
fontWeight: FontWeight.bold,
),
),
),
BlocSelector<CaptureCubit, CaptureCubitState, bool>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using BlocSelector here to scope rebuilds

Copy link
Contributor

Choose a reason for hiding this comment

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

i get it, all fine

selector: (state) => state.isScoreSelected,
builder: (context, isScoreSelected) {
return AnimatedOpacity(
duration: const Duration(milliseconds: 1500),
opacity: isScoreSelected ? 1.0 : 0.0,
child: Column(
children: const [
AnswerChips(),
SizedBox(height: 32),
_SubmitButton(),
],
),
],
),
);
},
),
const SizedBox(height: 55),
// TODO(Jan-Stepien): implement Need Help button
Expand All @@ -109,51 +96,81 @@ class CaptureView extends StatelessWidget {
}
}

class _SubmitButton extends StatelessWidget {
const _SubmitButton({Key? key}) : super(key: key);

@override
Widget build(BuildContext context) {
final canSubmit = context.select(
(CaptureCubit cubit) => cubit.state.canSubmit,
);
return ElevatedButton(
onPressed:
canSubmit ? () => context.read<CaptureCubit>().submitResult() : null,
child: const Padding(
padding: EdgeInsets.symmetric(
horizontal: 72,
vertical: 12,
),
child: Text(
Texts.submit,
style: TextStyle(
fontWeight: FontWeight.bold,
),
),
),
);
}
}

class CaptureScoreSelector extends StatelessWidget {
const CaptureScoreSelector({Key? key, this.radius = 30}) : super(key: key);

final double radius;

@override
Widget build(BuildContext context) {
final theme = Theme.of(context);
final maxScore = context.select(
(CaptureCubit cubit) => cubit.state.maxScore,
);
return Row(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
children: List.generate(
context.read<CaptureCubit>().state.maxScore,
(index) => AnimatedContainer(
duration: const Duration(milliseconds: 500),
height: radius * 2,
decoration: BoxDecoration(
shape: BoxShape.circle,
color:
context.select((CaptureCubit cubit) => cubit.state.score) - 1 ==
index
? NpsColors.colorSecondary
: NpsColors.colorGrey5,
),
child: TextButton(
onPressed: () =>
context.read<CaptureCubit>().selectScore(score: index + 1),
style: ButtonStyle(
shape: MaterialStateProperty.all<CircleBorder>(
const CircleBorder(),
),
alignment: Alignment.center,
maxScore,
(index) {
final isSelected = context.select(
(CaptureCubit cubit) => cubit.state.score - 1 == index,
);
return AnimatedContainer(
duration: const Duration(milliseconds: 500),
height: radius * 2,
decoration: BoxDecoration(
shape: BoxShape.circle,
color:
isSelected ? NpsColors.colorSecondary : NpsColors.colorGrey5,
),
child: Text(
(index + 1).toString(),
style: Theme.of(context).textTheme.subtitle1?.copyWith(
color: context.select(
(CaptureCubit cubit) => cubit.state.score,
) -
1 ==
index
? NpsColors.colorWhite
: NpsColors.colorPrimary1,
),
child: TextButton(
onPressed: () {
context.read<CaptureCubit>().selectScore(score: index + 1);
},
style: ButtonStyle(
shape: MaterialStateProperty.all<CircleBorder>(
const CircleBorder(),
),
alignment: Alignment.center,
),
child: Text(
(index + 1).toString(),
style: theme.textTheme.subtitle1?.copyWith(
color: isSelected
? NpsColors.colorWhite
: NpsColors.colorPrimary1,
),
),
),
),
),
);
},
),
);
}
Expand Down Expand Up @@ -214,37 +231,37 @@ class AnswerChips extends StatelessWidget {
alignment: WrapAlignment.center,
children: List<List<Widget>>.generate(
chips.length,
(index) => <Widget>[
ActionChip(
onPressed: () =>
context.read<CaptureCubit>().chipToogled(index: index),
padding: const EdgeInsets.symmetric(
horizontal: 16,
vertical: 12,
),
label: Text(
chips[index],
style: TextStyle(
fontWeight: FontWeight.bold,
color: context
.watch<CaptureCubit>()
.state
.chipIndexes
.contains(index)
? NpsColors.colorWhite
: NpsColors.colorPrimary1,
(index) {
final isSelected = context.select(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is to reduce of 'watch rebuilds' ? from 2 to 1?

Copy link
Contributor Author

@felangel felangel Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I apologize if I was unclear when I said scope the watches as low as possible. I meant scope them to the lowest BuildContext. Having multiple watches on the same context is less efficient than a single top level watch.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense,
Im super greatfull for a review, all your comments are very helpful

(CaptureCubit cubit) => cubit.state.chipIndexes.contains(index),
);
return <Widget>[
ActionChip(
onPressed: () {
context.read<CaptureCubit>().chipToogled(index: index);
},
padding: const EdgeInsets.symmetric(horizontal: 16, vertical: 12),
label: Text(
chips[index],
style: TextStyle(
fontWeight: FontWeight.bold,
color: isSelected
? NpsColors.colorWhite
: NpsColors.colorPrimary1,
),
),
backgroundColor:
isSelected ? NpsColors.colorSecondary : NpsColors.colorGrey5,
),
backgroundColor:
context.watch<CaptureCubit>().state.chipIndexes.contains(index)
? NpsColors.colorSecondary
: NpsColors.colorGrey5,
),
const SizedBox(width: 8),
],
).reduce(
(value, element) => [...value, ...element],
),
const SizedBox(width: 8),
];
},
).reduce((value, element) => [...value, ...element]),
);
}
}

extension on CaptureCubitState {
bool get isScoreSelected => score != -1;
bool get canSubmit => score != -1 && chipIndexes.isNotEmpty;
}
Comment on lines +264 to +267
Copy link
Contributor

Choose a reason for hiding this comment

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

hows this better than getters on cubit? i dont really get it

Copy link
Contributor Author

@felangel felangel Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Extension in the UI indicates that this is purely presentation logic that is "syntactic sugar". I recommend avoiding exposing public methods to query state information from the cubit directly because ideally the cubit should only be accessed in order notify of changes in the UI. Getters on the cubit are error prone because they don't trigger rebuilds when the computed value changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, it wont trigger changes and we want to keep the state as a complete definition, i see that, thanks