Fix popup UI; Added scroll pickers #17
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the UI for various pickers in the subscription management screen, migrating from a custom modal container design to CupertinoActionSheet with scroll pickers. It also adds a showSearch parameter to the currency picker widget and bumps the app version from 1.0.2+8 to 1.1.1+10.
Changes:
- Migrated currency, billing cycle, purchase date, and tag pickers from list-based selection to scroll wheel pickers wrapped in CupertinoActionSheet
- Added
showSearchparameter toshowCurrencyPickerto optionally hide the search field - Removed dependencies on
showCurrencyPickerandshowTagPickerin add_subscription_sheet.dart and implemented inline scroll pickers - Created a new
_TagOptionhelper class for tag picker data representation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| pubspec.yaml | Version bumped to 1.1.1+10 and removed blank line |
| lib/presentation/widgets/currency_picker.dart | Added showSearch parameter and migrated to CupertinoActionSheet |
| lib/presentation/widgets/add_subscription_sheet.dart | Replaced external picker functions with inline scroll pickers using CupertinoPicker and FixedExtentScrollController |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (currency) => currency.code.toUpperCase() == _currencyCode.toUpperCase(), | ||
| ); | ||
| if (tempIndex < 0) tempIndex = 0; | ||
| final controller = FixedExtentScrollController(initialItem: tempIndex); |
There was a problem hiding this comment.
The FixedExtentScrollController is created but never disposed. This could lead to a memory leak. Consider disposing the controller after the modal is dismissed, for example by calling controller.dispose() after the await statement.
| final cycle = await showCupertinoModalPopup<BillingCycle>( | ||
| final initialIndex = _orderedCycles.indexOf(_cycle); | ||
| var tempIndex = initialIndex < 0 ? 0 : initialIndex; | ||
| final controller = FixedExtentScrollController(initialItem: tempIndex); |
There was a problem hiding this comment.
The FixedExtentScrollController is created but never disposed. This could lead to a memory leak. Consider disposing the controller after the modal is dismissed, for example by calling controller.dispose() after the await statement.
| ); | ||
| if (initialIndex < 0) initialIndex = 0; | ||
| var tempIndex = initialIndex; | ||
| final controller = FixedExtentScrollController(initialItem: tempIndex); |
There was a problem hiding this comment.
The FixedExtentScrollController is created but never disposed. This could lead to a memory leak. Consider disposing the controller after the modal is dismissed, for example by calling controller.dispose() after the await statement.
| if (!mounted) return; | ||
| final selected = widget.currencies[tempIndex]; | ||
| setState(() => _currencyCode = selected.code.toUpperCase()); | ||
| state.didChange(_currencyCode); |
There was a problem hiding this comment.
The picker always applies the selected value even when the user presses the cancel button. The old implementation only updated the value when a selection was explicitly confirmed. This changes the user experience - now scrolling in the picker and then pressing cancel will still apply the scrolled-to value. Consider tracking whether the user confirmed or cancelled the selection, and only applying the change on confirmation.
| if (!mounted) return; | ||
| final selected = _orderedCycles[tempIndex]; | ||
| setState(() => _cycle = selected); | ||
| state.didChange(selected); |
There was a problem hiding this comment.
The picker always applies the selected value even when the user presses the cancel button. The old implementation only updated the value when a selection was explicitly confirmed. This changes the user experience - now scrolling in the picker and then pressing cancel will still apply the scrolled-to value. Consider tracking whether the user confirmed or cancelled the selection, and only applying the change on confirmation.
| if (!mounted) return; | ||
| if (result == -1) { | ||
| setState(() => _selectedTagId = null); | ||
| state.didChange(null); | ||
| } else { | ||
| setState(() => _selectedTagId = result); | ||
| state.didChange(result); | ||
| } | ||
| final selected = options[tempIndex]; | ||
| setState(() => _selectedTagId = selected.tagId); | ||
| state.didChange(selected.tagId); |
There was a problem hiding this comment.
The picker always applies the selected value even when the user presses the cancel button. The old implementation (using showTagPicker) only updated the value when a selection was explicitly confirmed. This changes the user experience - now scrolling in the picker and then pressing cancel will still apply the scrolled-to value. Consider tracking whether the user confirmed or cancelled the selection, and only applying the change on confirmation.
| cancelButton: CupertinoActionSheetAction( | ||
| onPressed: () => Navigator.of(context).pop(), | ||
| child: Text(localizations.done), |
There was a problem hiding this comment.
The date picker always applies the selected value even when the user presses the cancel button. The old implementation had a "Done" button to confirm the selection. This changes the user experience - now scrolling the date picker and then pressing cancel will still apply the scrolled-to date. Consider tracking whether the user confirmed or cancelled the selection, and only applying the change on confirmation.
No description provided.