-
Notifications
You must be signed in to change notification settings - Fork 277
feat(Settings): redesign to new vue components #7682
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
Conversation
|
Looks great! Is this ready for testing? Can you also rebase to the latest calendar version, it will fix some of the CI errors |
|
Just a small note before reviewing the whole thing: I updated the mockup in the issue to use the (future) NcFormBoxSelectNative component for the default reminder and density. |
cf794a0 to
06d1fec
Compare
Yep, it should work... CI tho... |
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.
Looks really nice :)
A couple of issues:
- Boxes and groups are not used correctly. "Availability settings" has a low border radius and the CalDAV settings are separated. The discussion here might be useful.
- Default reminder should be above Simple event editor in Editing unless there was a reason not to put it there.
- The keyboard shortcuts should use the new component as well. It's fine if it will be in a followup PR, but it should be done too.
This is the styling we went with for the components themselves, but indeed, dark mode was definitely not thought out very much and should probably be adjusted. |
And I think this one should be fixed by nextcloud-libraries/nextcloud-vue#7851 |
This should fix it, it's a Nextcloud vue thing
Fixed!, will push in a second |
These are using |
06d1fec to
e9fc7d3
Compare
Inside there is |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7682 +/- ##
==========================================
- Coverage 11.10% 11.09% -0.01%
==========================================
Files 328 328
Lines 62162 62174 +12
Branches 919 919
==========================================
Hits 6901 6901
- Misses 55125 55137 +12
Partials 136 136
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a5fc44d to
9074c7e
Compare
|
@GVodyanov could you please update the screenshots? |
src/components/AppNavigation/Settings/SettingsAttachmentsFolder.vue
Outdated
Show resolved
Hide resolved
Updated! |
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.
Thank you, looks good :)
The only thing now is that keys for keyboard shortcuts should be capital letters, like on a real keyboard.
5288a7e to
ef13cb6
Compare
ShGKme
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.
Approving from the component usage side
src/components/AppNavigation/Settings/SettingsAttachmentsFolder.vue
Outdated
Show resolved
Hide resolved
ef13cb6 to
5220466
Compare
src/components/AppNavigation/Settings/SettingsAttachmentsFolder.vue
Outdated
Show resolved
Hide resolved
Signed-off-by: Grigory V <[email protected]> # Conflicts: # package-lock.json
5220466 to
4ec261c
Compare
ShGKme
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.
Still approved
|
Look good, Merge it when you're ready |
|
🎉 |
|
/backport to stable6.0 |
|
/backport to stable6.0 please |


Fix #7558
The only difference from the design spec is the NcSelect not being grey and not having the label "inside" the border, but those require nextcloud vue changes.
Updated screenshots:


