-
Notifications
You must be signed in to change notification settings - Fork 244
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(editors): redesign editors #5675
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5675 +/- ##
============================================
- Coverage 24.48% 24.19% -0.29%
Complexity 423 423
============================================
Files 243 243
Lines 10954 11005 +51
Branches 1803 1825 +22
============================================
- Hits 2682 2663 -19
- Misses 8272 8342 +70
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
91b7a0b
to
77e8265
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.
it looks like in the screenshots, and no errors were shown. 🥳
ah, i have this warning while building:
|
Yeah, that is likely related to a recent bump of nextcloud/moment #5699. I'll have a look next week but it should not be related to this PR. I'll also fix js unit tests ASAP. |
The warning is unrelated (also on main) and will be fixed in #5722. |
0ca10aa
to
201da78
Compare
Blocked until the nextcloud/vue 8 migration was merged. |
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 is super super cool! :D really great :) Some minor comments:
- The title of the event could be more eye catching, so it could be bold and even in bigger font size
- All the icons on the left could be center aligned. Currently the icon of the details are slightly more to the left, the icon of the calendar on top is a bit to the right, and the icons in the top and bottom section in the sidebar are slightly misaligned. All these should ideally be center aligned :) (suggestion: add some left margin for the details elements in the card and the top section of the sidebar)
- There should also be an icon for the date time, it could be the event MDI icon
Nice-to-haves that are not in the original mockups:
- if there is no recurrence, the "No recurrence" wording can be changed to "Does not repeat" to keep it simple
- Use a edit icon for the edit button
- Use a done icon for the save button
Regarding the accept/reject/tentative buttons:
- You are right, placing it directly as the last row looks off. I am not sure about placing such bold buttons in the middle of the card either. How about placing it as the second-last row and making the edit button a tertiary button?
- Hide accept/decline/reject buttons in edit mode
Or alternatively no full width buttons, but all right justified
What do you think? Also pinging the rest of @nextcloud/designers for your thoughts on the accept buttons :)
Feedback has been addressed. This PR is still waiting for the nc/vue upgrade.
Signed-off-by: Richard Steinmetz <[email protected]>
4bf540c
to
3369fc8
Compare
Oh, I just realized we don't have the illustrations on the sidebar anymore :( |
Fix #3543
Screenshots
Simple Editor
Sidebar Editor
Invitations
I moved the invitation response button below the date and time fields. They looked a bit awkward at the bottom.
TODO