Skip to content

feat: drift album page #19564

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat: drift album page #19564

wants to merge 12 commits into from
+1,173 −25

Conversation

alextran1502
Copy link
Contributor

  • feat: drift album page
  • feat: page renderred

@wuzihao051119
Copy link
Collaborator

You spelled drift_album.page.dart as drift_ablum.page.dart.

@alextran1502 alextran1502 marked this pull request as ready for review June 27, 2025 06:08
shenlong-tanwen
shenlong-tanwen reviewed Jun 27, 2025
View reviewed changes
mobile/lib/providers/infrastructure/remote_album.provider.dart
dependencies: [remoteAlbumServiceProvider],
);

class RemoteAlbumState {
Copy link
Member

Choose a reason for hiding this comment

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

TBH, We do not need this as a provider at all. The idea for a provider is to use it when you have a common state that is shared across widgets. This one, however, is used only in the Albums page. Routes opened from this page do not need access to this so ideally, it would be best to move all the logic inside this provider to the Widget state and remove this one.

TLDR:

  • State shared between widgets across different page / routes -> Providers
  • State used in only one page -> Widget State

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 think we should follow the same convention everywhere for an expected development flow. Widget > Provider > Service > Repository. Additionally, if you include these logic within the page file, it will get pretty long with all the UI component code, then you would need to think about how to break out those logic into a separate file, so eventually we would come full cycle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wuzihao051119 wuzihao051119 wuzihao051119 left review comments

@shenlong-tanwen shenlong-tanwen Awaiting requested review from shenlong-tanwen

@jrasm91 jrasm91 Awaiting requested review from jrasm91

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
changelog:skip 📱mobile
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alextran1502 @wuzihao051119 @shenlong-tanwen