-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add configuration for full res on zoom and full res always for public shares #672
Conversation
General comment: would it not make sense to make this a global default instead? I.e. instead of being specific to public links, use the setting as the default for all users (unless the user changes it) and apply the same for public links. |
I did start implementing it that way but then thought that a separate setting for public links was nice. i.e. I can have always on loading but public links that may be handed out to many viewers can be turned off. Or something like that. Completely happy to change it if you would prefer. |
That's the default behavior. The case I'm thinking about is the opposite: if public links have full res turned on by default, then logged in users probably want this on by default anyway (with the option of turning it off). |
Are you imagining that the user can turn off full res images if they are on globally, and also turn them on if they are off globally? I think part of the reason I went with this is because I was struggling to imagine the UI for that. |
In latest commit I have made the admin settings global and added an explicit "Override" setting for the user settings. Let me know if you can think of a more elegant way. |
@update:checked="updateOverrideGlobalFullRes" | ||
type="switch" | ||
> | ||
{{ t('memories', 'Override the global settings for loading full size images') }} |
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.
Don't think we need this; seems too complex for the average user. Just a default provided by the admin is good enough.
Anyway, this looks good overall, will make some adjustments before merging.
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 added this because I wasn't sure how otherwise a user could turn off the default, but I assume you are thinking that if an admin has turned it on then there's not a good reason that an individual user would need to turn it off?
Also would you like me to remove this or will you be doing it?
@pulsejet Do you need me to do anything for this to be merged? |
Merged, thanks! 🎉 |
Add seperate full res configuration for public shares to the admin settings.
Fixes: #662