-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
feat: sliver appbar and snap scrubbing #19446
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
Conversation
16e1dcb
to
31b01d6
Compare
final isScreenLandscape = | ||
MediaQuery.orientationOf(context) == Orientation.landscape; |
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.
final isScreenLandscape = | |
MediaQuery.orientationOf(context) == Orientation.landscape; | |
final isScreenLandscape = context.orientation == Orientation.landscape; |
pinned: pinned, | ||
snap: snap, | ||
expandedHeight: expandedHeight, | ||
backgroundColor: context.colorScheme.surfaceContainer, |
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.
Should we do surface
instead? This will have a similar background as the current implementation
backgroundColor: context.colorScheme.surfaceContainer, | |
backgroundColor: context.colorScheme.surface, |
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 had it as surface
because we couldn't hide the app bar in the current implementation. With the sliver implementation, I like the better segmentation of the app bar to the body
void _scrollToLayoutSegment(int layoutSegmentIndex) { | ||
final layoutSegment = widget.layoutSegments[layoutSegmentIndex]; | ||
final maxScrollExtent = _scrollController?.position.maxScrollExtent; | ||
final viewportHeight = _scrollController?.position.viewportDimension; | ||
|
||
final targetScrollOffset = layoutSegment.startOffset; | ||
final centeredOffset = targetScrollOffset - (viewportHeight! / 4) + 100; | ||
|
||
_scrollController?.jumpTo(centeredOffset.clamp(0.0, maxScrollExtent!)); | ||
} |
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.
This seems off. The segments offset cannot be directly translated to the viewports offset. When pulling it down and running locally, I could see the snapping missing a lot of month segments. But since this is still in dev, I am fine with merging it as is and we can iterate on this later.
return InkWell( | ||
onTap: () => showDialog( | ||
context: context, | ||
useRootNavigator: false, | ||
builder: (ctx) => const ImmichAppBarDialog(), | ||
), | ||
borderRadius: BorderRadius.circular(12), | ||
child: Badge( |
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.
return InkWell( | |
onTap: () => showDialog( | |
context: context, | |
useRootNavigator: false, | |
builder: (ctx) => const ImmichAppBarDialog(), | |
), | |
borderRadius: BorderRadius.circular(12), | |
child: Badge( | |
return IconButton( | |
onPressed: () => showDialog( | |
context: context, | |
useRootNavigator: false, | |
builder: (ctx) => const ImmichAppBarDialog(), | |
), | |
icon: Badge( |
return InkWell( | ||
onTap: () => context.pushRoute(const BackupControllerRoute()), | ||
borderRadius: BorderRadius.circular(12), | ||
child: Badge( |
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.
return InkWell( | |
onTap: () => context.pushRoute(const BackupControllerRoute()), | |
borderRadius: BorderRadius.circular(12), | |
child: Badge( | |
return IconButton( | |
onPressed: () => context.pushRoute(const BackupControllerRoute()), | |
icon: Badge( |
Padding( | ||
padding: const EdgeInsets.only(right: 12), | ||
child: IconButton( | ||
onPressed: () { | ||
showDialog( | ||
context: context, | ||
builder: (context) => const CastDialog(), | ||
); | ||
}, | ||
icon: Icon( | ||
isCasting ? Icons.cast_connected_rounded : Icons.cast_rounded, | ||
), | ||
), | ||
), | ||
if (showUploadButton) | ||
const Padding( | ||
padding: EdgeInsets.only(right: 20), | ||
child: _BackupIndicator(), | ||
), | ||
const Padding( | ||
padding: EdgeInsets.only(right: 20), | ||
child: _ProfileIndicator(), |
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.
You don't need to add padding after switching to IconButton
label: Container( | ||
decoration: BoxDecoration( | ||
color: Colors.black, |
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.
label: Container( | |
decoration: BoxDecoration( | |
color: Colors.black, | |
label: DecoratedBox( | |
decoration: BoxDecoration( | |
color: context.colorScheme.onSecondary, |
label: Container( | ||
width: widgetSize / 2, | ||
height: widgetSize / 2, | ||
decoration: BoxDecoration( |
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.
label: Container( | |
width: widgetSize / 2, | |
height: widgetSize / 2, | |
decoration: BoxDecoration( | |
label: SizedBox( | |
width: widgetSize / 2, | |
height: widgetSize / 2, | |
child: DecoratedBox( | |
decoration: BoxDecoration( |
border: Border.all( | ||
color: context.colorScheme.outline.withValues(alpha: .3), | ||
), | ||
borderRadius: BorderRadius.circular(widgetSize / 2), |
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.
borderRadius: BorderRadius.circular(widgetSize / 2), | |
borderRadius: const BorderRadius.all(Radius.circular(widgetSize / 2)), |
Change all BorderRadius.circular
to const BorderRadius.all(Radius.circular(..))
Icons.cloud_off_rounded, | ||
size: 9, | ||
color: iconColor, | ||
semanticLabel: 'backup_controller_page_backup'.tr(), |
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.
semanticLabel: 'backup_controller_page_backup'.tr(), | |
semanticLabel: 'backup_controller_page_backup'.t(context: context), |
Use t extension for all
} | ||
|
||
Widget? _getBackupBadgeIcon(BuildContext context, WidgetRef ref) { | ||
final BackUpState backupState = ref.watch(backupProvider); |
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.
final BackUpState backupState = ref.watch(backupProvider); | |
final BackUpState backupState = ref.watch(backupProvider); |
You're calling ref.watch(backupProvider)
inside _getBackupBadgeIcon
, which is invoked by build()
. According to Riverpod docs, this is an anti-pattern—ref.watch
should be called directly inside build()
to ensure proper and predictable widget rebuilding. Please move final backupState = ref.watch(backupProvider);
to the top of build()
.
if (isEnableAutoBackup) { | ||
if (backupState.backupProgress == BackUpProgressEnum.inProgress) { | ||
return Container( | ||
padding: const EdgeInsets.all(3.5), | ||
child: CircularProgressIndicator( | ||
strokeWidth: 2, | ||
strokeCap: StrokeCap.round, | ||
valueColor: AlwaysStoppedAnimation<Color>(iconColor), | ||
semanticsLabel: 'backup_controller_page_backup'.tr(), | ||
), | ||
); | ||
} else if (backupState.backupProgress != | ||
BackUpProgressEnum.inBackground && | ||
backupState.backupProgress != BackUpProgressEnum.manualInProgress) { | ||
return Icon( | ||
Icons.check_outlined, | ||
size: 9, | ||
color: iconColor, | ||
semanticLabel: 'backup_controller_page_backup'.tr(), | ||
); | ||
} | ||
} | ||
|
||
if (!isEnableAutoBackup) { |
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.
You can use a switch
expression to make the code more concise and readable than using if-else statements
return Container( | ||
padding: const EdgeInsets.all(3.5), | ||
child: CircularProgressIndicator( | ||
strokeWidth: 2, | ||
strokeCap: StrokeCap.round, | ||
valueColor: AlwaysStoppedAnimation<Color>(iconColor), | ||
semanticsLabel: 'backup_controller_page_backup'.tr(), | ||
), | ||
); |
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.
Remove the Container and its padding—since you're already using padding, the CircularProgressIndicator doesn't appear. Tesed on Iphone SE
return Container( | |
padding: const EdgeInsets.all(3.5), | |
child: CircularProgressIndicator( | |
strokeWidth: 2, | |
strokeCap: StrokeCap.round, | |
valueColor: AlwaysStoppedAnimation<Color>(iconColor), | |
semanticsLabel: 'backup_controller_page_backup'.tr(), | |
), | |
); | |
return CircularProgressIndicator( | |
strokeWidth: 2, | |
strokeCap: StrokeCap.round, | |
valueColor: AlwaysStoppedAnimation<Color>(iconColor), | |
semanticsLabel: 'backup_controller_page_backup'.tr(), | |
); |
Thanks, @dvbthien, for the review. I will probably open a separate PR for that specific sliver app bar refactor. |
No description provided.