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

[WIP] MVP Images #9455

Closed
wants to merge 3 commits into from
Closed

[WIP] MVP Images #9455

wants to merge 3 commits into from

Conversation

flexsurfer
Copy link
Member

@flexsurfer flexsurfer commented Nov 16, 2019

MVP Images [WIP] Take a picture is not implemented yet

Upload an image to infura and send a hash

DEMO https://www.youtube.com/watch?v=6Vhv-j6n8Po

Choose Upload Send
image image image

@flexsurfer flexsurfer requested a review from errorists November 16, 2019 10:16
@flexsurfer flexsurfer requested a review from a team as a code owner November 16, 2019 10:16
@flexsurfer flexsurfer self-assigned this Nov 16, 2019
@ghost
Copy link

ghost commented Nov 16, 2019

Pull Request Checklist

  • Docs: Updated the documentation, if affected
  • Docs: Added or updated inline comments explaining intention of the code
  • Tests: Ensured that all new UI elements have been assigned accessibility IDs
  • Tests: Signaled need for E2E tests with label, if applicable
  • Tests: Briefly described what was tested and what platforms were used
  • UI: In case of UI changes, ensured that UI matches Figma
  • UI: In case of UI changes, requested review from a Core UI designer
  • UI: In case of UI changes, included screenshots of implementation

@status-im-auto
Copy link
Member

status-im-auto commented Nov 16, 2019

Jenkins Builds

Click to see older builds (7)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ b7a423f #1 2019-11-16 10:25:56 ~9 min ios 📦ipa 📲
✔️ b7a423f #1 2019-11-16 10:30:24 ~13 min android 📦apk 📲
✔️ b7a423f #1 2019-11-16 10:30:28 ~13 min android-e2e 📦apk 📲
✔️ b7a423f #1 2019-11-16 10:30:49 ~14 min macos 📦dmg
✔️ b7a423f #1 2019-11-16 10:30:53 ~14 min linux 📦App
✔️ b7a423f #1 2019-11-16 10:31:48 ~15 min windows 📦exe
b7a423f #2 2019-11-28 12:55:20 ~17 sec ios 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 67083a4 #3 2019-11-29 09:53:38 ~7 min ios 📦ipa 📲
✔️ 67083a4 #2 2019-11-29 09:59:29 ~13 min android-e2e 📦apk 📲
✔️ 67083a4 #2 2019-11-29 09:59:36 ~13 min android 📦apk 📲
✔️ 67083a4 #4 2019-12-02 10:55:18 ~7 min ios 📦ipa 📲
67083a4 #1 2020-03-31 13:42:11 ~35 sec android 📄log
67083a4 #1 2020-03-31 13:43:31 ~1 min android-e2e 📄log
✔️ ce00e55 #2 2020-03-31 14:05:08 ~9 min ios 📦ipa 📲
✔️ ce00e55 #2 2020-03-31 14:06:44 ~10 min android-e2e 📦apk 📲
✔️ ce00e55 #2 2020-03-31 14:06:52 ~10 min android 📦apk 📲

@errorists
Copy link
Contributor

👏A👏W👏E👏S👏O👏M👏E👏

I treat this as a proof of concept for the purpose of UI correctness, @flexsurfer please let me know if you'd like me to point out any UI styling issues, there's only a handful.

However, one huge UX request : please allow images to be previewed and send with their correct aspect ratio preserved and not cropped in a square. I think this needs to be addressed before I feel comfortable shipping this MVP.

this is as good illustration as any of what I mean
Photos -_ Camera access

I guess it can be left outside the scope for now but the first immediate thing to address would be to add full screen image previews once tapped on an image in chat

Photos -_ Camera Preview

[wrapper {:keys [content] :as message}]
[wrapper message
[react/image {:style {:margin-vertical 10 :width 140 :height 140 :border-radius 8}
:source {:uri (str "https://ipfs.infura.io/ipfs/" (:hash content))}}]])
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not impose IPFS in the URL, we can still validate the link client side if we want to have only IPFS domains, but otherwise changing this would be breaking. Rather I would pass the full url, and then we can only show if starts-with ipfs or something.
Also markdown supports images, so it's possible to embed images in text content if we'd like.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cammellos you mean unfurl a link that leads to an image? or place an inline image next to regular text? I would say it's not something we want, images should always be placed separately as their own message bubble in chat.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's what I meant (regarding markdown, not the URL, that's a protocol thing), thanks for the reply.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a temporary implementation, there will be a multihash instead ipfs hash, we don't want to send any web2 links or images, multihash might be used for ipfs or swarm or our own storage later

@flexsurfer flexsurfer changed the title MVP Images [WIP] [WIP] MVP Images Nov 20, 2019
@flexsurfer flexsurfer closed this Mar 11, 2020
@flexsurfer flexsurfer reopened this Mar 31, 2020
@rachelhamlin
Copy link
Contributor

Blocking us from shipping:

  • Image doesn't appear right now because a status-go update is required
  • Are images maintaining their aspect ratio now?
  • If I add text after choosing an image, the image is then removed and only the text is sent. Is it possible to send both at once? 1. Select an image; 2. Add text; 3. Send both at once.
  • Take image flow is not complete. Tapping on the camera icon to reverse the camera will lead to 1) Appearance settings, and 2) take a photo and make it your profile picture. 😂 Nice workaround for custom photos! Now I can't delete it.

@errorists
Copy link
Contributor

If I add text after choosing an image, the image is then removed and only the text is sent. Is it possible to send both at once? 1. Select an image; 2. Add text; 3. Send both at once.

yes, that's something made possible with the 'Final' (needs better naming) design. I - maybe - could make it work with MVP but it wouldn't be pretty

@rachelhamlin
Copy link
Contributor

@errorists is it better to disable the text field when an image is inserted? User can still 'x' it out. But if they type, they remove their image. It wasn't 100% clear to me that the image weren't still being sent along w/ my text, until I sent it.

Just a thought, not a deal breaker for me though.

@errorists
Copy link
Contributor

Probably not, I mean when you focus the input, you cancel out of the image sending dialog. to bring it back you need to tap the image icon again, so it's either one or the other.

It's the MVP flavour of this design for which I apologise but in my excuse there's a better, fully formed one waiting there

@errorists
Copy link
Contributor

since it doesn't hurt to ask, @Ferossgp would you care to estimate how much longer it would take you to add the UI features in the full design? I mean the image composition menu that has a horizontally scrollable gallery built in and a camera viewfinder? and the image queue above the input that allows for queuing multiple images, pasting images and adding text / composing replies to an image?

@Ferossgp
Copy link
Contributor

Ferossgp commented Apr 3, 2020

@errorists UI components are not complex, but given the fact that we should interact with some native library like Camera Roll and camera preview, I would expect complexity and possible pitfalls on that. It also depends on how multiple images would be handled. If there will be no limitation on one image in MVP then it should not be hard (Meaning that message with image structure will not limit only one URL). I'm not sure at this time how to better implement paste image cause I've never worked with this in RN. I would allocate 2/3 days without paste image

@rachelhamlin
Copy link
Contributor

@errorists no need to apologize, I think it works just fine for now. 2/3 days ain't bad for the bulk of the remainder, but for the MVP let's just ensure these user requirements are met:

  • Aspect ratio is maintained for images in chat
  • Take image flow is working

cc @flexsurfer

@flexsurfer flexsurfer closed this Apr 20, 2020
@hesterbruikman
Copy link
Contributor

@flexsurfer should we reopen this in light of status-im/specs#69 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants