Skip to content
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

Show vcard in messages #3840

Merged
merged 6 commits into from
Jun 1, 2024
Merged

Show vcard in messages #3840

merged 6 commits into from
Jun 1, 2024

Conversation

nicodh
Copy link
Contributor

@nicodh nicodh commented May 21, 2024

resolves #3829

@nicodh
Copy link
Contributor Author

nicodh commented May 21, 2024

Something seems to be wrong with the prettier check: local check shows no errors, the errors displayed here like
"app.asar" are not in the code base?

@nicodh nicodh marked this pull request as draft May 21, 2024 06:28
Comment on lines 553 to 556
<div
className={classNames('avatar', styles.avatar)}
aria-label={displayName}
>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to use the same Avatar component as chatlist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, but then we have to change the line <img className='content' src={'file://' + avatarPath} /> there, since the profileImage is base64 encoded. Will do that later...

@Simon-Laux
Copy link
Member

Something seems to be wrong with the prettier check: local check shows no errors, the errors displayed here like
"app.asar" are not in the code base?

was a prettier (formatting error) on main branch that got carried into your branch, it was complaining about double quotes vs single quotes. nothing to do with your pr.

the error now about misplaced console.log is also about an issue on the main branch which was solved in the meantime, somehow the ci is not run anymore on the main branch, maybe related to renaming it, I'll investigate.

@nicodh nicodh marked this pull request as ready for review May 30, 2024 08:53
@nicodh
Copy link
Contributor Author

nicodh commented May 30, 2024

Display in stage, replies, drafts etc. uses just the currently provided stuff from core, which might be improved in future

@@ -586,6 +589,11 @@ export default function Message(props: {
const hasText = text !== null && text !== ''
const fileMime = (!isSetupmessage && message.fileMime) || null
const isWithoutText = isMediaWithoutText(fileMime, hasText, message.viewType)
const showAttachment = (message: T.Message) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

showAttachment as function sounds like an action, maybe rename it to sth like shouldShowAttachment or make it a variable/const instead of a function?

Copy link
Member

@Simon-Laux Simon-Laux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works nicely, I just have some code suggestions. before merging we would need to create an issue for the composer preview, so that todo does not get lost

CHANGELOG.md Outdated
Comment on lines 34 to 46
- show VCard attachement as VCard in message list
- add contact from VCard & start chat on click
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be rebased / not be in the old/already released version

@Simon-Laux Simon-Laux self-assigned this Jun 1, 2024
@Simon-Laux Simon-Laux force-pushed the ndh/task-3829-viewtype-vcard branch from 3211415 to 39ce931 Compare June 1, 2024 18:30
@Simon-Laux Simon-Laux merged commit 4ca80f7 into main Jun 1, 2024
7 checks passed
@Simon-Laux Simon-Laux deleted the ndh/task-3829-viewtype-vcard branch June 1, 2024 18:34
@adbenitez adbenitez mentioned this pull request Jun 5, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Viewtype::Vcard
4 participants