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

Closes #58 #66

Merged
merged 14 commits into from May 23, 2021
Merged

Closes #58 #66

merged 14 commits into from May 23, 2021

Conversation

ryg-git
Copy link
Contributor

@ryg-git ryg-git commented May 11, 2021

No description provided.

@aguilaair aguilaair changed the title Issue 58 Closes #58 May 12, 2021
@aguilaair aguilaair linked an issue May 12, 2021 that may be closed by this pull request
Copy link
Collaborator

@aguilaair aguilaair left a comment

Choose a reason for hiding this comment

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

Looking good. Just a few notes to improve the end user experience.

Thanks for your awesome work!

lib/widgets/sendMessage.dart Show resolved Hide resolved
lib/widgets/sendMessage.dart Outdated Show resolved Hide resolved
lib/widgets/sendMessage.dart Outdated Show resolved Hide resolved
@ryg-git
Copy link
Contributor Author

ryg-git commented May 12, 2021

@aguilaair thanks for the review, I will implement the needful changes.

@aguilaair
Copy link
Collaborator

aguilaair commented May 12, 2021

Ok, thanks again for putting your time into this feature! I appreciate it.

@ryg-git ryg-git marked this pull request as ready for review May 15, 2021 18:06
Copy link
Collaborator

@aguilaair aguilaair left a comment

Choose a reason for hiding this comment

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

Looks good, I'll do some minor changes and see of I can implement support for Windows and MacOS than i think it is ready to merge!

Thanks for you help 😃

Copy link
Collaborator

@aguilaair aguilaair left a comment

Choose a reason for hiding this comment

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

This implementation currently does not work with Web. dart:io is used and is therefore is incompatible with web. Additionally, the file picker will always return a Uint8List in web as paths aren't supported. If you have a chance to fix it it would be great!

@ryg-git
Copy link
Contributor Author

ryg-git commented May 16, 2021

I will fix it as soon as I can thanks for the review @aguilaair

@aguilaair
Copy link
Collaborator

Thank you for your effort! I really appreciate it.

@ryg-git ryg-git marked this pull request as draft May 18, 2021 13:30
@ryg-git
Copy link
Contributor Author

ryg-git commented May 18, 2021

@aguilaair I have tested attachment upload and download on android device and on web (chrome), I am not able to test it on iOS device if it is possible for you to test it on iOS please do although I don't think it will not work on iOS. If you want anything changed please let me know.

Thank you!

@ryg-git ryg-git marked this pull request as ready for review May 18, 2021 14:09
@aguilaair
Copy link
Collaborator

Great! I'll check it out ASAP

@aguilaair aguilaair self-requested a review May 23, 2021 12:21
@aguilaair
Copy link
Collaborator

Looks great! Made a few tewaks to show the body even if an attachment is shown and also tweaked some alerts to be more neutral themed.

Thanks for your effort. I believe this can now be merged and it will be included in the next release!

@aguilaair aguilaair merged commit 7acbef9 into papercups-io:main May 23, 2021
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.

Image/attachment upload
2 participants