-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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(contactsmenu): Show user status #40852
Conversation
requesting early design review based on the before/after screenshots I think the feature is done. I only have to adjust/write tests |
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 good! small cosmetic changes:
- "Show all contacts" remove ellipsis and text in bold so it looks like a button
- size of avatar could be 44px (ignore if already the case, it looks smaller than 44px to me)
- increase space between avatar and text by 2px
- decrease space between main text and subline by 2px
Basically similar to the styling of a list item in, for eg. the contacts app.
- Also, if clicking on the item opens the profile anyway, I'd say we don't need a separate profile button, it could just be a 3 dot menu
Do you want me to remove the profile action or just move it into the actions menu? The profile is an action like the others, but it has higher priority. The first action in inline and in many cases that is the profile action. |
Ah okay, I understood, so for emails that would be a mail action? How about we make Talk the priority action and keep profile in the 3 dot menu. That way, when you open the contact search you see users whom you can Talk to and email addresses that you can send mails to, and if you click on the user you would go to their profile anyway. My idea is that you are probably looking for a contact to do something specific like chat or email more than view the profile. What do you think? |
I'm fine with either. You decide. We don't have to change this here, though. The sole goal of this PR is to add the status line :) |
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 nice! :) I’d agree with @nimishavijay that having "Talk" as the primary action makes more sense as it’s a more direct action.
However then clicking the name/status part of the person should do the same to not have 4 different click areas (avatar, name, action icon, action menu), and "Profile" has to be put into the action menu – so your call if this should be in here or in a separate pull request @ChristophWurst.
Let's do these changes separately 👍 |
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.
Only checked PHP
8bbe444
to
167501a
Compare
167501a
to
5f44e85
Compare
Red CI unrelated. Cypress is random and postgres timed out. |
I hovered the button to stop automerge and when I clicked the page jumped and my click went to the upate button. Great. Just four more hours. |
fae6ae8
to
5f44e85
Compare
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
5f44e85
to
ac168cf
Compare
Summary
Show the user status for system contacts if they have a status set. Show the email address otherwise.
TODO
Checklist