Implement message link inside message #2005
Conversation
Passing run #2428 ↗︎
Details:
Review all test suite changes for PR #2005 ↗︎ |
|||||||||||||||
There was a problem hiding this comment.
Nice, it seems to work really well on Desktop, but on mobile it doesn't work at all.
Please try with grunt dev --tunnel and then with a cellphone + Desktop.
On desktop you'll have URLs that look like this: http://localhost:3000/....
You'll need to replace that with the URL that localtunnel gives you, e.g. https://gi5209.loca.lt/... in order to test on mobile.
Having tested in mobile, it works fine to me. Both in browser and PWA. @SebinSong, can you please try to review this PR? Especially, test how message link works in mobile. |
|
@Silver-IT I will review this PR once I have time this week.
you can test it yourself using EDIT: I just realised you mentioned you already did. ;) |
Thanks, @SebinSong. When you test this PR, do not make mistake in creating message links. That's because hostnames would be different; one would be cc: @taoeffect |
That's too obvious... but thnx for a reminder. |
|
Need to consider the Issue #2041 too. This is the comment to me. |
| this.updateScroll(messageHashToScroll, Boolean(mhash)).then(() => { | ||
| if (mhash) { | ||
| this.$router.replace({ query: {} }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
It's to remove mhash in queries after scroll to and highlight the message with mhash. Since we use only mhash as query in the Chat page, so just made query empty.
| if (entry.tagName === 'ROUTER') { | ||
| const hasChildren = Array.isArray(entry.children) | ||
| const route = entry.attributes.route && JSON.parse(entry.attributes.route) | ||
| const href = route && this.$router.resolve(route).href | ||
| return createElement( | ||
| 'a', | ||
| { | ||
| class: 'link', | ||
| attrs: { | ||
| href | ||
| }, | ||
| on: { | ||
| click: (e) => { | ||
| route && this.$router.push(route) | ||
| e?.preventDefault() | ||
| }, | ||
| touchhold: (e) => { | ||
| href && sbp('okTurtles.events/emit', OPEN_TOUCH_LINK_HELPER, href) | ||
| e?.preventDefault() | ||
| } | ||
| } | ||
| }, | ||
| hasChildren | ||
| ? entry.children.map(child => recursiveCall(child)) | ||
| : undefined | ||
| ) | ||
| } else if (entry.tagName) { |
There was a problem hiding this comment.
This is to find in-app links which are created by makeInAppLinkElement function and create elements in real.
There was a problem hiding this comment.
@Silver-IT , it looks like this additional block has some duplicate codes with the else if (entry.tagName) { ... } below.
I would DRY this by including the logic in there like,
if (entry.tagName) {
const hasChildren = Array.isArray(entry.children)
const isRouter = entry.tagName === 'ROUTER'
const elName = isRouter ? 'a' : entry.tagName.toLowerCase()
const opts = isRouter
? {
// options you created above
}
: {
// default options
}
....
return createElement(
elName,
opts,
hasChildren
? entry.children.map(child => recursiveCall(child))
: undefined
)
}
frontend/views/pages/GroupChat.vue
Outdated
| } else if (mhash) { | ||
| this.$refs.chatMain?.scrollToMessage(mhash).then(() => { | ||
| this.$router.replace({ query: {} }) | ||
| }) |
There was a problem hiding this comment.
When user clicks the link to the message of the current chatroom, it is handled in here.
|
Hi @Silver-IT , Just roughly read through this PR and (correct me if I'm wrong) what some of logics here does is apparently, it intercepts the link markdown and
could you give some explanation on why adding step 2. & 3. here was necessary? Cuz below block added in this PR feels redundant (Or it's that I don't have a full understanding on the context of the issue.) on: {
click: (e) => {
route && this.$router.push(route)
e?.preventDefault()
},
touchhold: (e) => {
href && sbp('okTurtles.events/emit', OPEN_TOUCH_LINK_HELPER, href)
e?.preventDefault()
}
}
|
@SebinSong, that's very nice catch. I've just realised those two steps seems not be necessary. Actually, the logic was implemented before you finished improving the way to render messages which contains markdown and mentions. But now, when your code is already implemented, I don't think those two steps are needed any more. 👍 And also I think we can use the |
|
@Silver-IT I just tried your fix in both desktop and in my mobile phone and it works well for both.
And yes, for anywhere it can be useful and also pls feel free to enhance the component to suit your need. |
There was a problem hiding this comment.
Nice work @Silver-IT!
I tested this and it seems to work almost perfectly.
A few remaining issues:
On mobile, clicking "report the error" shows wrong view
Flow errors
Running "exec:flow" (exec) task
Error ------------------------------ frontend/views/containers/chatroom/chat-mentions/RenderMessageWithMarkdown.js:40:27
Cannot assign object literal to `routerOptions.route` because property `route` is missing in object literal [1].
[prop-missing]
frontend/views/containers/chatroom/chat-mentions/RenderMessageWithMarkdown.js:40:27
40| routerOptions.route = { path, query }
^^^^^
References:
frontend/views/containers/chatroom/chat-mentions/RenderMessageWithMarkdown.js:30:31
30| const routerOptions = { isInAppRouter: false }
^^^^^^^^^^^^^^^^^^^^^^^^ [1]
Error ------------------------------ frontend/views/containers/chatroom/chat-mentions/RenderMessageWithMarkdown.js:41:27
Cannot assign `this.$router.resolve(...).href` to `routerOptions.href` because property `href` is missing in object
literal [1]. [prop-missing]
frontend/views/containers/chatroom/chat-mentions/RenderMessageWithMarkdown.js:41:27
41| routerOptions.href = this.$router.resolve(routerOptions.route).href
^^^^
References:
frontend/views/containers/chatroom/chat-mentions/RenderMessageWithMarkdown.js:30:31
30| const routerOptions = { isInAppRouter: false }
^^^^^^^^^^^^^^^^^^^^^^^^ [1]
Found 2 errors
The Flow check failed!
There was a problem hiding this comment.
Almost ready to merge, still a flow error:
Running "exec:flow" (exec) task
Error ------------------------------------------------------------------ frontend/controller/actions/identity.js:801:102
Cannot call `deleteResult.map` because property `map` is missing in possibly uninitialized variable [1].
[incompatible-use]
frontend/controller/actions/identity.js:801:102
801| console.error('[gi.actions/identity/removeFiles] Some CIDs could not be deleted', deleteResult.map((r, i) => r.status === 'rejected' && toDelete[i]).filter(Boolean))
^^^
References:
frontend/controller/actions/identity.js:766:9
766| let deleteResult, toDelete
^^^^^^^^^^^^ [1]
Found 1 error
The Flow check failed!
There was a problem hiding this comment.
Ah, that flow error is from master, and @SebinSong fixes it in #2090
Summary of Changes