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
feat(server, web): album order preference #9135
base: main
Are you sure you want to change the base?
Conversation
256202a
to
223e6a7
Compare
Deploying immich with Cloudflare Pages
|
bf804f1
to
cd16415
Compare
cd16415
to
ae17ea0
Compare
1f7dc5b
to
d14e16e
Compare
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 like this much more now! Thank you :)
if (order === AssetOrder.PREFERENCE) { | ||
return enumToOrder[auth.user.preferedAlbumOrder]; | ||
} |
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 does not need to be in the outer if. You can simply move it in front of it :)
@@ -40,7 +41,9 @@ export class TimelineService { | |||
private async buildTimeBucketOptions(auth: AuthDto, dto: TimeBucketDto): Promise<TimeBucketOptions> { | |||
const { userId, ...options } = dto; | |||
let userIds: string[] | undefined = undefined; | |||
|
|||
if (dto?.order === AssetOrder.PREFERENCE) { | |||
options.order = auth.user.preferedAlbumOrder as unknown as AssetOrder; |
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.
What is wrong with those types so that you need to cast it to unknown
first?
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.
It's because we have
export enum AssetOrderPreference {
ASC = 'asc',
DESC = 'desc',
}
and
export enum AssetOrder {
ASC = 'asc',
DESC = 'desc',
PREFERENCE = 'preference',
}
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.
AssetOrderPreference
is a subset of AssetOrder
though, right? I don't understand why TS doesn't get this :D
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.
Extending an enum would be cool ...
Why don't you convert this to ready for review after you have removed the per album/user sorting setting. I think we can keep the default sort order per person, and default to that setting for any new albums, but we don't need per user sort settings for each album. |
Martabal reverted that, no? Afaict this PR now only has (1) default setting per user and (2) sorting order per album (not per album per user). |
Yep, I think that's how we decided it to be 🤔 |
The goal is to have a new user settings to choose the default album order (the option is saved in db). The options are
Backward
andForward
.Screenshots
User settings :