-
Notifications
You must be signed in to change notification settings - Fork 138
feat: introduce pinia for admin settings #4621
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Elizabeth Danzberger <[email protected]>
fc82d0a to
f24d64f
Compare
|
@juliusknorr Do you think we should ignore the remote font config warning for now? Didn't we discuss that, because we can have these configuration files of our own now, we can always ensure there is one? |
|
Curently we cannot verify this anyways as it is a setting in coolwsd.xml and we cannot check that. Mid-term collabora planned to remove this configuration option as far as I understood and pass over the fonts without any configuration required, so I'd say it is fine to ignore that, yes. |
Signed-off-by: Elizabeth Danzberger <[email protected]>
f24d64f to
6c9d5ad
Compare
|
@nextcloud/designers I have uploaded a screen recording, can I get a design review on this please? :) |
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.
I'd say generally it's fine, but instead of the red trash button, a tertiary one (icon only) is enough?
Also @marcoambrosini @susnux what would be cool is a nice component for lists so this is consistent across Nextcloud. If we want to get fancy, this could be a grid/tiled view with font and font name below each other, sort of laid out like the tiles we use for the settings of Dashboard.
| --> | ||
|
|
||
| <template> | ||
| <NcSettingsSection :name="t('richdocuments', 'Custom Fonts')" |
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.
| <NcSettingsSection :name="t('richdocuments', 'Custom Fonts')" | |
| <NcSettingsSection :name="t('richdocuments', 'Custom fonts')" |
Detail: Should be sentence case, like below for "Available fonts"
| data-cy="newFontInput" | ||
| @change="selectFile"> | ||
|
|
||
| <table> |
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 would not use a table for this, generally tables are bad for a11y and especially in this case where you do not have a table header.
Instead it probably makes more sense to use a list for this, semantically, and style it like you want.
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.
Thanks for the feedback, I really appreciate any a11y tips I can get!
We already have |
Summary
Introduces Pinia in an effort to organize the admin settings a bit instead of having everything in one massive component.
Screen Recording
Custom Fonts
admin-settongs-redesign-custom-fonts.mp4
TODO
Checklist