Generate a link for @-Mentionings #462

roschaefer · 2019-04-12T23:04:57Z

Pullrequest

I'm not able to provide proper testing of the Editor component. When I unskip this test case I get TypeError: this.view.root.getSelection is not a function. I tried to find working component tests in their repo but unfortunately the entire library seems to be untested 🙀 there are no traces in package.json or issue list. I would be super glad if sb. could help me out on writing component tests for the Editor of if sb. has ideas how to resolve the above error.

Regarding the implementation: This PR sticks extremely close to their example code. It's basically copy-paste from here. That means I added tippy.js which gives animated tooltips and fuse.js which does fuzzy search in the list of suggestions. I can see the value of fuse.js but I would question the need for tippy. @appinteractive are tooltips and the suggestions menu sth. easy to implement? Opinions? If we implement it ourselves, we could at least test some parts of the editor.

After understanding how tiptap works, my task was just to create a custom Mention node. The original code which I overrode is Mention.js.

Oh yeah and one last thing. I'm fetching the entire user collection (id+slug) here.

Issues

fixes Generate a link from @-mention in editor #447

Checklist

None

How2Test

Try to write or edit a post and enter @ in the content field.

Todo

None

Full stack trace:

● Editor.vue › mount › given a piece of text › renders

    TypeError: this.view.root.getSelection is not a function

      337 |     // if it's the first item, navigate to the last one
      338 |     upHandler() {
    > 339 |       this.navigatedUserIndex =
          |                     ^
      340 |         (this.navigatedUserIndex + this.filteredUsers.length - 1) %
      341 |         this.filteredUsers.length
      342 |     },

      at SelectionReader.domChanged (node_modules/prosemirror-view/dist/index.js:2033:28)
      at EditorView.updateState (node_modules/prosemirror-view/dist/index.js:4370:92)
      at Editor.setContent (node_modules/tiptap/dist/tiptap.common.js:1127:17)
      at VueComponent.handler (components/Editor/index.vue:339:21)
      at VueComponent.Vue.$watch (node_modules/vue/dist/vue.runtime.common.dev.js:4926:12)
      at createWatcher (node_modules/vue/dist/vue.runtime.common.dev.js:4883:13)
      at initWatch (node_modules/vue/dist/vue.runtime.common.dev.js:4865:7)
      at initState (node_modules/vue/dist/vue.runtime.common.dev.js:4628:5)
      at VueComponent.Vue._init (node_modules/vue/dist/vue.runtime.common.dev.js:4981:5)
      at new VueComponent (node_modules/vue/dist/vue.runtime.common.dev.js:5128:12)
      at createComponentInstanceForVnode (node_modules/vue/dist/vue.runtime.common.dev.js:3277:10)
      at init (node_modules/vue/dist/vue.runtime.common.dev.js:3108:45)
      at createComponent (node_modules/vue/dist/vue.runtime.common.dev.js:5952:9)
      at createElm (node_modules/vue/dist/vue.runtime.common.dev.js:5899:9)
      at VueComponent.patch [as __patch__] (node_modules/vue/dist/vue.runtime.common.dev.js:6449:7)
      at VueComponent.Vue._update (node_modules/vue/dist/vue.runtime.common.dev.js:3927:19)
      at VueComponent.updateComponent (node_modules/vue/dist/vue.runtime.common.dev.js:4048:10)
      at Watcher.get (node_modules/vue/dist/vue.runtime.common.dev.js:4459:25)
      at new Watcher (node_modules/vue/dist/vue.runtime.common.dev.js:4448:12)
      at mountComponent (node_modules/vue/dist/vue.runtime.common.dev.js:4055:3)
      at VueComponent.Object.<anonymous>.Vue.$mount (node_modules/vue/dist/vue.runtime.common.dev.js:8386:10)
      at mount (node_modules/@vue/test-utils/dist/vue-test-utils.js:8649:21)
      at Wrapper (components/Editor/spec.js:21:45)
      at Object.it (components/Editor/spec.js:39:19)

@appinteractive

@appinteractive we have `yarn run lint` on our build server. I would say this is enough to enforce linting. I get slowed down a little during development. Instead of runing `yarn run lint --fix` every time I save, I would like to `yarn run lint --fix` all in one before I commit.

See https://github.com/scrumpy/tiptap/blob/master/examples/Components/Routes/Suggestions/index.vue

All components should consist of a folder with these three files: ``` README.d index.vue spec.js ``` When you import components, omit the `index.vue`. That helps to `git grep` for component names.

@mattwr18

Note that we don't create duplicate notifications. I made use of the behaviour of XSS-middleware: It removes all css classes from `<a>` anchors. Because notifications rely on a css class `mention` which gets removed in a later middleware, this gives us a nice behaviour for re-notifications without creating duplicates. The downside is that it creates dependencies between middlewares and it's not that obvious at all. cc @mattwr18 @ulfgebhardt @appinteractive @Tirokk

This will help some people not to loose data after accidently clicking on the user @-mentioning.

…ning_link_in_editor

mattwr18 · 2019-04-17T17:29:22Z

backend/src/middleware/notifications/extractMentions.spec.js

+    })
+
+    describe('given a link with .mention class', () => {
+      it('extracts ids', () => {


I pulled down the code and added a went to /post/create, and was playing around with the updated Editor, it seems really nice to me.

One thing I did was to add a mention, and then inspected and looked at the link and it doesn't seem to be added a .mention class as I was suspecting based on the tests, but maybe this is removed in xssMiddleware as your comment 👇 suggests?

<a href="/profile/u2" target="_blank">@bob-der-baumeister</a>

mattwr18

One of the things I noticed about this PR is the editor has some features I hadn't noticed before

there are validations on creating a Post with a title, which must be between 3 to 64 characters.
The mentions drop down is a nice addition.
I did notice that I am able to mention myself, which might be a refactor?

Overall, unless anyone else comes in with reasons we shouldn't get this merged, I am approving the changes as they are a great improvement and it is well tested.

roschaefer · 2019-04-17T18:07:25Z

@mattwr18 unfortunately the frontend is not well tested. For the reasons above. Good catch that you are able to mention yourself 😉 I noticed this and used it for manual testing. Maybe we don't even have to prevent this and you can happily notifiy yourself if you want. 😆 Let's wait for sb. to ask for the prevention.

Regarding the .mention class, see the comment here. Not very obvious but I hope this OK.

roschaefer force-pushed the 447-generate_mentioning_link_in_editor branch 2 times, most recently from f9903b3 to 359fbd2 Compare April 15, 2019 23:37

roschaefer added 10 commits April 16, 2019 01:39

Scaffold test for Editor.vue

11d217f

Convenience: Run seeds from root folder

e94e81e

Copy+paste from tiptap example

2529c62

See https://github.com/scrumpy/tiptap/blob/master/examples/Components/Routes/Suggestions/index.vue

Pass entire user collection to Editor

f760f37

Follow file naming convention

b70e5be

All components should consist of a folder with these three files: ``` README.d index.vue spec.js ``` When you import components, omit the `index.vue`. That helps to `git grep` for component names.

Customize Mention node based on Superclass

d87a0d5

Fix lint

b21df87

Move ContributionForm in dedicated folder

4efb834

Order users by name

7492e6b

roschaefer force-pushed the 447-generate_mentioning_link_in_editor branch from 359fbd2 to f5cf3d9 Compare April 16, 2019 00:06

roschaefer added 8 commits April 16, 2019 02:08

Use slugs instead of names for mentions

f5cf3d9

Follow component directory strucutre in backend

23a461d

Change backend test to parse html not plain text

f30da03

Implement parsing of mention links

b2d72ae

Implement right order of notifications middleware

aa3762a

Open mentioning link in new tab

e247d25

This will help some people not to loose data after accidently clicking on the user @-mentioning.

Merge remote-tracking branch 'origin/master' into 447-generate_mentio…

ab0d10a

…ning_link_in_editor

roschaefer requested review from appinteractive, ulfgebhardt and mattwr18 April 16, 2019 20:25

mattwr18 reviewed Apr 17, 2019

View reviewed changes

mattwr18 approved these changes Apr 17, 2019

View reviewed changes

roschaefer changed the title ~~[WIP] Generate a link for @-Mentionings~~ Generate a link for @-Mentionings Apr 17, 2019

Lulalaby self-requested a review April 17, 2019 19:13

Lulalaby approved these changes Apr 17, 2019

View reviewed changes

roschaefer merged commit f733efc into master Apr 18, 2019

roschaefer deleted the 447-generate_mentioning_link_in_editor branch April 18, 2019 10:19

ulfgebhardt added the feature label Apr 19, 2019

ulfgebhardt added this to To Do in Human-Connection via automation Apr 19, 2019

ulfgebhardt added this to the Sprint: Inanna milestone Apr 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate a link for @-Mentionings #462

Generate a link for @-Mentionings #462

roschaefer commented Apr 12, 2019 •

edited

mattwr18 Apr 17, 2019

mattwr18 left a comment

roschaefer commented Apr 17, 2019

Generate a link for @-Mentionings #462

Generate a link for @-Mentionings #462

Conversation

roschaefer commented Apr 12, 2019 • edited

Pullrequest

Issues

Checklist

How2Test

Todo

mattwr18 Apr 17, 2019

Choose a reason for hiding this comment

mattwr18 left a comment

Choose a reason for hiding this comment

roschaefer commented Apr 17, 2019

roschaefer commented Apr 12, 2019 •

edited