-
Notifications
You must be signed in to change notification settings - Fork 172
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 shared items and events for each contact #3670
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3670 +/- ##
==========================================
+ Coverage 1.80% 2.06% +0.25%
==========================================
Files 113 89 -24
Lines 6133 5379 -754
Branches 1505 1523 +18
==========================================
Hits 111 111
+ Misses 5901 5147 -754
Partials 121 121
☔ View full report in Codecov by Sentry. |
47b09cf
to
a187fb3
Compare
Big screen view. Looks like this. I will make the spaces between app shares smaller. But otherwise, will look like this.
|
The Anything shared with the same group of people … is strange for events shared with this contact IMO. If this is coming from nc/vue, we might want to add an option to hide or replace the text. |
Super nice! It looks great :)
I agree with this, in fact I would say we don't even need a subline if we name the headings appropriately.
Some more feedback/questions:
For smaller screens, I would agree with your placement in the screenshot, below the rest of the contact info. Since people use contacts apps on their phones a lot I would not remove it completely, but it can go at the bottom :) |
Feedback from the rest of the team:
cc @jancborchardt :) @GretaD it would be great if the previews could make it in as they are a pretty important part of this feature. If there is something much more urgent on your todo list then all good, but otherwise the previews would be great to have :) |
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.
Tested and works so far.
There are some design issues:
- The related resource panel does not wrap to the right column. It is rendered below the contacts form.
- Panels with errors should be hidden. They emit an event
@has-error
which could be leveraged to achieve this. The state of each panel could be saved to a boolean and then the panel could be hidden viav-if
if the error is true for a given panel.
There are 2 more apps, calendar and decks. What header do you propose there? Right now is Calendar events and Deck cards.
If the related resource changes to support preview and the changes we need on the nc/vue are merged and tested today, i can try and make it work for tomorrow(deadline), but this needs some work in different apps and libraries and we might not have the time to do it perfectly and not easily breakable design wise. |
@GretaD @nimishavijay maybe to simplify, we can go with "[…] with you" for all headings? So:
And no subline. Then it's clear it's things you have in common, in clear language. (Otherwise it sounded like you can see their shares.) |
I think this was caused by the error message, because it was too long and not wrapped. Now we hide it when the app is not activated, so the screen will break when the screen is smaller than 1600px wide. |
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.
Tested and works. My previous review was addressed.
Unfortunately, we can only merge this after updating nc-vue to 8 :(
794b068
to
178700b
Compare
Signed-off-by: greta <[email protected]>
c80a0fe
to
43de764
Compare
fixes #3364