Skip to content

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

Merged
merged 13 commits into from
Jun 25, 2025
Merged

feat: sliver appbar and snap scrubbing #19446

merged 13 commits into from
Jun 25, 2025

Conversation

alextran1502
Copy link
Member

No description provided.

@alextran1502 alextran1502 changed the title feat: sliver appbar feat: sliver appbar and snap scrubbing Jun 23, 2025
Comment on lines 19 to 20
final isScreenLandscape =
MediaQuery.orientationOf(context) == Orientation.landscape;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final isScreenLandscape =
MediaQuery.orientationOf(context) == Orientation.landscape;
final isScreenLandscape = context.orientation == Orientation.landscape;

pinned: pinned,
snap: snap,
expandedHeight: expandedHeight,
backgroundColor: context.colorScheme.surfaceContainer,
Copy link
Member

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

Suggested change
backgroundColor: context.colorScheme.surfaceContainer,
backgroundColor: context.colorScheme.surface,

Copy link
Member Author

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

Comment on lines 273 to 282
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!));
}
Copy link
Member

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.

Comment on lines +135 to +142
return InkWell(
onTap: () => showDialog(
context: context,
useRootNavigator: false,
builder: (ctx) => const ImmichAppBarDialog(),
),
borderRadius: BorderRadius.circular(12),
child: Badge(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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(

Comment on lines +186 to +189
return InkWell(
onTap: () => context.pushRoute(const BackupControllerRoute()),
borderRadius: BorderRadius.circular(12),
child: Badge(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return InkWell(
onTap: () => context.pushRoute(const BackupControllerRoute()),
borderRadius: BorderRadius.circular(12),
child: Badge(
return IconButton(
onPressed: () => context.pushRoute(const BackupControllerRoute()),
icon: Badge(

Comment on lines +69 to +90
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(),
Copy link
Contributor

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

Comment on lines +143 to +145
label: Container(
decoration: BoxDecoration(
color: Colors.black,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label: Container(
decoration: BoxDecoration(
color: Colors.black,
label: DecoratedBox(
decoration: BoxDecoration(
color: context.colorScheme.onSecondary,

Comment on lines +190 to +193
label: Container(
width: widgetSize / 2,
height: widgetSize / 2,
decoration: BoxDecoration(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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().

Comment on lines +222 to +245
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) {
Copy link
Contributor

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

Comment on lines +224 to +232
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(),
),
);
Copy link
Contributor

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

Suggested change
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(),
);

@alextran1502
Copy link
Member Author

Thanks, @dvbthien, for the review. I will probably open a separate PR for that specific sliver app bar refactor.

@alextran1502 alextran1502 merged commit 05064f8 into main Jun 25, 2025
47 checks passed
@alextran1502 alextran1502 deleted the sliver-app-bar branch June 25, 2025 01:02
@alextran1502 alextran1502 mentioned this pull request Jun 30, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants