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

Send an image message via Matrix #1154

Merged
merged 11 commits into from
Nov 7, 2023
Merged

Send an image message via Matrix #1154

merged 11 commits into from
Nov 7, 2023

Conversation

ratik21
Copy link
Collaborator

@ratik21 ratik21 commented Nov 3, 2023

What does this do?

Adds the ability to send an image message while matrix is enabled.

Screen.Recording.2023-11-04.at.4.20.02.AM.mov

@ratik21 ratik21 requested a review from a team November 3, 2023 22:57
@@ -357,7 +353,7 @@ export class MessageInput extends React.Component<Properties, State> {
}

get allowFileAttachment() {
return !featureFlags.enableMatrix && !this.props.isEditing;
return !this.props.isEditing;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we allowing any type of file now or just images?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

only images 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but enabling this will allow a user to select / drag any type of file, right? As in, do we need more checks to prevent other files?

Copy link
Collaborator Author

@ratik21 ratik21 Nov 6, 2023

Choose a reason for hiding this comment

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

yes, it would. That request will just fail because we only support sending the images for now. I left it thinking sending files are gonna be a recent follow up. I can look into adding more checks, to only allow images to be "sent" via the UI for now. Let me know

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you see how it fails? What does the user experience? If they're getting ok feedback then it's fine but we can't just let things get into bad states.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how it fails. I think that's an okay feedback 🤔
image

Copy link
Contributor

@dalefukami dalefukami Nov 7, 2023

Choose a reason for hiding this comment

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

How does the user know why that failed? If I were a user I'd just keep trying.

Confirm with Kelly, maybe? See if that's good enough for now?

Is there not just a way to limit the drag/drop component to certain file types or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so... it was actualy pretty simple. Just needed to comment these out. 😅
image

added a note. We're only allowing images to be attached for now.

Copy link
Contributor

@dalefukami dalefukami left a comment

Choose a reason for hiding this comment

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

Let's just merge this but please follow up with the file type discussion to get it resolved one way or the other.

@ratik21 ratik21 force-pushed the ZOS-698-support-image-send branch from 6023ba9 to 1cf9c5f Compare November 7, 2023 16:20
@ratik21 ratik21 merged commit 7fc30b1 into main Nov 7, 2023
4 checks passed
@ratik21 ratik21 deleted the ZOS-698-support-image-send branch November 7, 2023 20:12
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.

2 participants