-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(Settings): Add section to select preset #54570
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(Settings): Add section to select preset #54570
Conversation
6a28012 to
5c49df0
Compare
jancborchardt
left a comment
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.
Very nice! See the 2 wording fixes as inline comments.
2 additional things/questions:
- The "Apply selected preset" is disabled in this case, will it be primary when enabled? For visibility it’s probably better to left-align it so it’s more visible. We only really right-align these buttons in modals.
- The checkbox is quite far to the right but I assume with nextcloud-libraries/nextcloud-vue#7352 this will look better. :)
apps/settings/src/components/SettingsPresets/PresetVisualisation.vue
Outdated
Show resolved
Hide resolved
apps/settings/src/components/SettingsPresets/PresetVisualisation.vue
Outdated
Show resolved
Hide resolved
|
The technical names for these values should also probably be monospace. Although I'm not sure we should even expose them so prominently? I feel like this is more so something that should be explained in documentation (also which setting does what and what exactly), which we could link to from here. |
6a7b1a6 to
75738e4
Compare
kra-mo
left a comment
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.
Really nice :)
The only two things I have to say now are that the apply button should probably be primary as @jancborchardt suggested, and that “Enforce password protection for shared documents” should be plural.
|
Oh, and the base localization is American English I think? So customise → customize. But correct me if I’m wrong, I’m not actually sure 😅 |
75738e4 to
532164d
Compare
|
Should be all good, thanks @kra-mo and @jancborchardt :) |
532164d to
9f28265
Compare
9f28265 to
3bbd884
Compare
|
/compile rebase |
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
3bbd884 to
673b8c9
Compare

Part of github.com//issues/51804