@sarahscott => [Messaging] WIP on updating messages connection/fragment #630

mzikherman · 2017-06-23T20:34:35Z

This uses the updated schema from artsy/metaphysics#680

It doesn't have the real Relay fragment yet...but the existing stuff does work (raw_text instead of snippet), and we now have is_from_user coming back from Metaphysics. I need to go back and do that in a more 'proper' way, but that should be transparent to Emission anyway.

mzikherman · 2017-06-23T20:34:57Z

package.json

@@ -29,7 +29,7 @@
    "tslint": "tslint",
    "sync-colors": "cd externals/elan && git fetch && git checkout origin/master && cp components/lib/variables/colors.json ../../data",
    "sync-externals": "npm run-script sync-schema && npm run-script sync-colors",
-    "sync-schema": "cd externals/metaphysics && git fetch && git checkout origin/master && yarn install && npm run dump-schema -- ../../data && rm -rf node_modules",
+    "sync-schema": "cd externals/metaphysics && git fetch && git checkout origin/message_details_from_impulse && yarn install && npm run dump-schema -- ../../data && rm -rf node_modules",


Will undo once Metaphysics is merged...

@mzikherman small tip:

You can run the following command locally to sync the schema of a metaphysics branch you're currently working on

cd <metaphysics_root_dir> && npm run dump-schema -- <path_to_emission_root>/data

mzikherman · 2017-06-23T20:36:17Z

src/lib/Containers/Conversation.tsx

@@ -61,14 +61,6 @@ const ComposerContainer = styled.View`
 `

 export class Conversation extends React.Component<RelayProps, any> {
-  isFromUser(message) {


mzikherman · 2017-06-23T20:36:30Z

src/lib/relay/config.tsx

@@ -9,6 +9,8 @@ if (Emission && Emission.useStagingEnvironment) {
  metaphysicsURL = "https://metaphysics-production.artsy.net"
 }

+metaphysicsURL = "http://localhost:5001"


Once Metaphysics is merged, will undo.

👍 - will need changing

mzikherman · 2017-06-27T01:01:23Z

So, I have this sorta working, w/o Relay fragments :(

(image previews, is_from_user, correct timestamps)

Where I'm at is that replacing the explicit queries with Message.getFragment(..) style (and presumably I'll refactor my ImagePreview into some Attachment.getFragment(..) stuff once I add PDFs, basically fails with strange and hard to debug errors.

So, I'm assuming that's a Relay issue, but for the life of me I couldn't figure it out. Some of the commits in artsy/metaphysics#680 were helping, I think (such as implementing the NodeInterface stuff), but still wasn't working.

Either way, let's pair! If I get this in a mergeable state we could even do that, and figuring out the proper fragment thing can be next.

sarahscott · 2017-06-27T08:01:38Z

src/lib/Containers/Conversation.tsx

-    const messageData = conversation.messages.edges.reverse().map(({ node }, index) => {
-      return {
-        senderName: this.isFromUser(node) ? conversation.from.name : partnerName,
-        key: index,


In ListViews and FlatLists, every item has to have a unique key. Usually we just use the id or __id of the object; whichever one is numeric.

Ah got it. That explains those warnings!

sarahscott · 2017-06-27T08:24:22Z

src/lib/Containers/Conversation.tsx

-      }
+    const messages = this.props.me.conversation.messages.edges.map(({ node }, index) => {
+      node.first_message = index === 0
+      node.key = node.id


sarahscott · 2017-06-27T08:27:12Z

So this now runs locally for me, although I haven't tried the ImagePreview fragment yet because I have no messages with attachments right now. @mzikherman does it work for you?

l2succes · 2017-06-27T09:36:22Z

package.json

@@ -29,7 +29,7 @@
    "tslint": "tslint",
    "sync-colors": "cd externals/elan && git fetch && git checkout origin/master && cp components/lib/variables/colors.json ../../data",
    "sync-externals": "npm run-script sync-schema && npm run-script sync-colors",
-    "sync-schema": "cd externals/metaphysics && git fetch && git checkout origin/master && yarn install && npm run dump-schema -- ../../data && rm -rf node_modules",
+    "sync-schema": "cd externals/metaphysics && git fetch && git checkout origin/message_details_from_impulse && yarn install && npm run dump-schema -- ../../data && rm -rf node_modules",


@mzikherman small tip:

You can run the following command locally to sync the schema of a metaphysics branch you're currently working on

cd <metaphysics_root_dir> && npm run dump-schema -- <path_to_emission_root>/data

l2succes · 2017-06-27T09:46:16Z

src/lib/Components/Inbox/Conversations/Previews/ImagePreview.tsx

+import OpaqueImageView from "../../../OpaqueImageView"
+
+const Container = styled.View`
+  borderWidth: 1


For consistency, use border-width style instead of camel case. Also, add semicolons at the end.

I'll add this package in my next PR to enforce css style
https://github.com/styled-components/stylelint-processor-styled-components

Make sure to add stylefmt too so that it's automated: #599

kebab-case please @mzikherman

Actually, don't bother - I will get this right now: https://twitter.com/rauchg/status/879752736467591168

in a separate PR

l2succes · 2017-06-27T09:47:20Z

src/lib/Components/OpaqueImageView.tsx

@@ -25,6 +25,8 @@ interface Props {
  /** Any additional styling for the imageview */
  style?: any

+  skipGemini?: boolean


what does skipGemini mean?

Ah sorry, didn't know we had a service named gemini 😅

lol, yea so the OpaqueImageView seems to have an overlay and then a resized on-the-fly image.

However...in testing yesterday it looks like the S3 URL's that are returned by Radiation which links to the attachment, error out when trying to resize with Gemini.

So this means that for now, it just renders the actual attachment (even for the tiny preview).

Later today I'll discuss with @joeyAghion / @ashkan18 the possibility of fixing up how Gemini resizes these (to avoid the error), or (possibly better), pre-render small thumbnails for image attachments. Then we could just serve those directly.

Makes sense

mzikherman · 2017-06-27T13:43:19Z

src/lib/Components/OpaqueImageView.tsx

@@ -81,6 +83,9 @@ export default class OpaqueImageView extends React.Component<Props, State> {

  imageURL() {
    const imageURL = this.props.imageURL
+    if (this.props.skipGemini) {


@l2succes so here, we just serve the URL right back, as opposed to trying to process on-the-fly.

It's just a temp hack for now just to get something working.

mzikherman · 2017-06-27T14:40:52Z

src/lib/Containers/Conversation.tsx

-        imageAttachmentUrl = item.attachments[0].download_url
-      }
-    }
+    const hasImageAttachment = item.attachments.length > 0 && item.attachments[0].content_type === "image/jpeg"


oo nice.

I'm going to update this for multiple attachments.

mzikherman · 2017-06-27T14:41:20Z

src/lib/Containers/Conversation.tsx

-                  is_from_user
-                  raw_text
-                  created_at
+                  id


wow, ok - I'm guessing this is what I was missing to get the fragment stuff working.

mzikherman · 2017-06-27T16:11:33Z

Hmm, I fixed up the specs so I think this is mergable, but still has a Danger issue @orta any ideas?

orta · 2017-06-27T16:22:05Z

It's because you're running from a fork - which we don't do on mobile repos. Can't deploy the PR nor run Danger without the secret env vars.

Peril will fix the danger issue, but not the latter, for now you should definitely add a CHANGELOG entry and consider it green.

mzikherman · 2017-06-27T16:31:39Z

It's because you're running from a fork - which we don't do on mobile repos. Can't deploy the PR nor run Danger without the secret env vars.

Oo I didn't realize that, I can re-open from a pushed branch?

Or happy to just do the CHANGELOG and consider it 💚 and do that for next time.

orta · 2017-06-27T16:36:55Z

I don't think you can change the repo remote - you can change branch, this is fine by me - whoever merges it should just do a quick check that everything other the PR updater, and Danger pass 👍

mzikherman · 2017-06-27T16:40:11Z

Eh, in that case I'll do that next time, it might be nice to have this PR merged, vs a new PR, to preserve some of the comments.

mzikherman · 2017-06-27T16:47:07Z

So I think this is mergable! And then we can merge artsy/metaphysics#680 to keep people in sync (people with in progress work will probably have to rebase once the Metaphysics schema hits staging).

mzikherman · 2017-06-27T17:15:07Z

@orta mind taking a peek? I think @sarahscott might be out on vacay.

I have some polish and other stuff, but this basically has things in a working state.

orta

This is good to merge, but only after undoing the change to a local metaphysics URL.

orta · 2017-06-27T17:16:46Z

Example/Podfile.lock

  Yoga:
-    :path: "../node_modules/react-native/ReactCommon/yoga"
+    :path: ../node_modules/react-native/ReactCommon/yoga


This is such a constant battle :D

orta · 2017-06-27T17:18:06Z

src/lib/Components/Inbox/Conversations/Message.tsx

          </Header>
          {this.props.artworkPreview && <ArtworkPreviewContainer>{this.props.artworkPreview}</ArtworkPreviewContainer>}
-          <BodyText>{this.props.message.body.split("\n\nAbout")[0]}</BodyText>
+          {this.props.imagePreview && <ImagePreviewContainer>{this.props.imagePreview}</ImagePreviewContainer>}
+          <BodyText>{this.props.message.raw_text.split("\n\nAbout")[0]}</BodyText>


Is this.props.message.raw_text.split("\n\nAbout")[0] worth verifying that the split will definitely work? Can't trust the server_too_ much ;)

So, this is actually going to change soon! Basically the sanitizing/formatting will be going upstream 🐟

👍 cool for now then

orta · 2017-06-27T17:18:55Z

src/lib/Components/Inbox/Conversations/Previews/ImagePreview.tsx

+import OpaqueImageView from "../../../OpaqueImageView"
+
+const Container = styled.View`
+  borderWidth: 1


kebab-case please @mzikherman

orta · 2017-06-27T17:19:16Z

src/lib/Components/OpaqueImageView.tsx

@@ -25,6 +25,8 @@ interface Props {
  /** Any additional styling for the imageview */
  style?: any

+  skipGemini?: boolean


Makes sense

orta · 2017-06-27T17:20:13Z

src/lib/Containers/Conversation.tsx

-  }
-
-  renderMessage(message) {
+  renderMessage({ item }) {


I forget about restructuring in params

orta · 2017-06-27T17:21:13Z

src/lib/relay/config.tsx

@@ -9,6 +9,8 @@ if (Emission && Emission.useStagingEnvironment) {
  metaphysicsURL = "https://metaphysics-production.artsy.net"
 }

+metaphysicsURL = "http://localhost:5001"


👍 - will need changing

mzikherman · 2017-06-27T17:35:13Z

Alrighty, updated with the Metaphysics override removed!

orta · 2017-06-27T17:39:03Z

OK - looks 👍

mzikherman assigned sarahscott Jun 23, 2017

mzikherman commented Jun 23, 2017

View reviewed changes

[Messaging] WIP on updating messages connection/fragment

870b9de

mzikherman force-pushed the message_fragmentz branch from 650c510 to 870b9de Compare June 23, 2017 20:35

mzikherman commented Jun 23, 2017

View reviewed changes

mzikherman added 3 commits June 26, 2017 16:03

More WIP Relay stuff

bce5a90

Try this Viewer thing

7bf4032

Working without relay fragmentz

0aea698

mzikherman force-pushed the message_fragmentz branch from c28efa3 to 0aea698 Compare June 26, 2017 21:40

lil image preview

e3212d7

sarahscott reviewed Jun 27, 2017

View reviewed changes

sarahscott added 2 commits June 27, 2017 10:18

[MSG] Fix up Relay fragments

cccfa06

[MSG] lets keep using me instead of viewer here

5c9d213

sarahscott reviewed Jun 27, 2017

View reviewed changes

[MSG] get tests passing

19bacdf

l2succes reviewed Jun 27, 2017

View reviewed changes

mzikherman commented Jun 27, 2017

View reviewed changes

mzikherman mentioned this pull request Jun 27, 2017

@l2succes => [Messaging] Updating messaging connection + attachments artsy/metaphysics#680

Merged

mzikherman added 2 commits June 27, 2017 11:19

Remove :log:

1205155

Update specs

0f1cf72

mzikherman added 2 commits June 27, 2017 12:34

Add changelog

9ec85ac

Merge remote-tracking branch 'upstream/master' into message_fragmentz

0856aad

orta approved these changes Jun 27, 2017

View reviewed changes

Remove metaphysics URL override

bacf861

orta merged commit d3b3449 into artsy:master Jun 27, 2017

alloy mentioned this pull request Dec 6, 2017

[ImageView] Get rid of skipGemini flag #875

Merged

@sarahscott => [Messaging] WIP on updating messages connection/fragment #630

@sarahscott => [Messaging] WIP on updating messages connection/fragment #630

Conversation

mzikherman commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzikherman commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahscott commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzikherman commented Jun 27, 2017

orta commented Jun 27, 2017 • edited Loading

mzikherman commented Jun 27, 2017

orta commented Jun 27, 2017

mzikherman commented Jun 27, 2017

mzikherman commented Jun 27, 2017

mzikherman commented Jun 27, 2017

orta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzikherman commented Jun 27, 2017

orta commented Jun 27, 2017

orta commented Jun 27, 2017 •

edited

Loading