Skip to content
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

refactor(mobile): Use widgets in Immich asset grid #7140

Merged
merged 4 commits into from
Feb 18, 2024

Conversation

martyfuhry
Copy link
Contributor

  • Replaced all buildThing with a Widget
  • This will ensure that the Flutter render engine can better cache and reuse existing Widgets
  • Performance is about the same
  • More boilerplate code, but good for future improvements and breaking the Widgets out a little more
  • More readable
  • Should have no regressions or changes
  • I profiled it and it seemed about the same as main

Copy link

cloudflare-workers-and-pages bot commented Feb 16, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 82845f9
Status: ✅  Deploy successful!
Preview URL: https://02cdd2ec.immich.pages.dev
Branch Preview URL: https://refactor-use-widgets-in-immi.immich.pages.dev

View logs

…the asset grid (#7142)

* Uses gradient image placeholders in memory lane and in the asset grid

* Changes to create placeholders in immmich image.thumbnail

* removed unused import
Comment on lines 27 to 35
final brightness = MediaQuery.platformBrightnessOf(context);
return Container(
width: width,
height: height,
margin: margin,
decoration: BoxDecoration(
gradient: LinearGradient(
colors: brightness == Brightness.light ? _brightColors : _darkColors,
begin: Alignment.topCenter,
Copy link
Member

@shenlong-tanwen shenlong-tanwen Feb 16, 2024

Choose a reason for hiding this comment

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

Since we have a theme switcher from within the app, the apps theme need not be in sync with the platforms theme. Kindly use the isDarkTheme getter from the context itself (context.isDarkTheme), which checks the brightness from the current themedata of the context. Since it is already a boolean, we need not explicitly check for equality when assigning colors to the gradient as well

With the way it is handled now, if the app is in dark theme and the platform theme is light, we'd display light colored placeholders instead of dark ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I've addressed this in my revision.

final bool selectionActive;
final Function(List<Asset>) selectAssets;
final Function(List<Asset>) deselectAssets;
final Function(List<Asset>) allAssetsSelected;
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Can we make the return type more explicit for the allAssetsSelected callback?

Suggested change
final Function(List<Asset>) allAssetsSelected;
final bool Function(List<Asset>) allAssetsSelected;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed this in my revision.

Copy link
Member

@shenlong-tanwen shenlong-tanwen left a comment

Choose a reason for hiding this comment

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

LGTM! The only thing that needs updating is the theme handling in placeholder

@alextran1502 alextran1502 merged commit 70e5feb into main Feb 18, 2024
25 checks passed
@alextran1502 alextran1502 deleted the refactor/use-widgets-in-immich-asset-grid branch February 18, 2024 03:31
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.

4 participants