Storybook #952

mattwr18 · 2019-07-02T19:35:28Z

🍰 Pullrequest

@appinteractive I put in a PR for this cause it is nice to play around with the Embeds here, and it would be nice to talk about merging in storybook

EDIT: fix #256

ToDos:

use provider.endpoint.schemes here: https://github.com/Human-Connection/Human-Connection/blob/master/backend/src/schema/resolvers/embeds/scraper.js#L43 using provider_url is just silly. This will fix the problem with youtu.be links

codecov · 2019-07-02T19:35:30Z

Codecov Report

Merging #952 into master will decrease coverage by 0.52%.
The diff coverage is 37.67%.

@@            Coverage Diff             @@
##           master     #952      +/-   ##
==========================================
- Coverage   37.37%   36.85%   -0.53%     
==========================================
  Files         213      225      +12     
  Lines        3061     3156      +95     
  Branches      161      160       -1     
==========================================
+ Hits         1144     1163      +19     
- Misses       1879     1955      +76     
  Partials       38       38

Impacted Files	Coverage Δ
webapp/layouts/default.vue	`0% <ø> (ø)`	⬆️
webapp/storybook/config.js	`0% <0%> (ø)`
webapp/components/PostCard/PostCard.story.js	`0% <0%> (ø)`
webapp/graphql/EmbedQuery.js	`0% <0%> (ø)`
webapp/components/Editor/Editor.story.js	`0% <0%> (ø)`
webapp/storybook/helpers.js	`0% <0%> (ø)`
webapp/storybook/webpack.config.js	`0% <0%> (ø)`
webapp/components/Editor/nodes/Link.js	`100% <100%> (ø)`
webapp/pages/post/_id/_slug/index.vue	`55.88% <100%> (+1.33%)`	⬆️
backend/src/middleware/xssMiddleware.js	`41.66% <100%> (+14.74%)`	⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80833b8...4324333. Read the comment docs.

mattwr18 · 2019-07-03T19:24:49Z

@Kachulio1 the build is broken cause of a webpack config issue... might you have a moment to have a look?

To workaround ``` Module build failed (from ./node_modules/css-loader/dist/cjs.js): ValidationError: CSS Loader Invalid Options options should NOT have additional properties ``` Some silly package of us is not up-to-date and still sends `exportOnlyLocals` to `css-loader` thus raising this error. I don't know which, so I'll downgrade css-loader to `2.x.x` which has the old API.

Some important commit messages: ``` Fix youtu.be not being embedded And also try to maintain the old behaviour matching `provider.provider_url`. ``` ``` Remove confusing code comments and obsolete code I discovered that the behaviour of no duplicate notifications being send out is caused by the frontend: When the editor reads html from the backend, it will parse hashtags and mentions as ordinary links, not as their respective nodes during editing. Also, we don't have to worry about duplicate ids being found: The cypher statement will implicitly suppress duplicate notification nodes for the same user. So let's remove the code to avoid confusing the next developer. ``` ``` Test editor.getHTML() I do this because I'm not able to test the content of `this.editor` from a wrapper of `vue-test-utils`. If I call `this.editor.getHTML` directly and use it as a computed property `renderedContent` to populate a `<div v-html="renderedContent" />` this will not work for the embeds. So, my current best bet is to test the editor object isolated from a real component. ;( ``` ``` Add core-js as explicit dependency Because of build errors on Travis. See: https://stackoverflow.com/a/55313456 Remove as soon as this issue is resolved: storybookjs/storybook#7591 ``` ``` Refactor: Keep Runtime-only builds See: https://vuejs.org/v2/guide/installation.html#Runtime-Compiler-vs-Runtime-only ```

mattwr18

Hey, this is really great @roschaefer... I had some questions/doubts below, and I'm trying to manually test it and noticed that peertube videos don't embed, I also couldn't find any gifs that embed.... maybe there is nothing we can do about that(?)

Maybe, we could have a pairing session yesterday when @Tirokk is available?

mattwr18 · 2019-08-01T05:46:22Z

backend/src/middleware/handleHtmlContent/notifications/extractMentionedUsers.spec.js

-        })
-      })
+  it('ignores links without .mention class', () => {
+    expect(extractMentionedUsers(contentWithLinks)).toEqual([])


contentWithLinks does contain the mention class, Oder?
I think what it is missing is the data-mention-id(?)

I renamed the variable to contentWithPlainLinks, it should ignore the links if they don't have data-mention-id . This is unambiguous, oder?

sorry, what I was questioning is the description ignores links without .mention class, cause the contentWithPlainLinks does include a mention class, but it is ignored for the reason you stated ☝️

I'm stupid, I meant the data-mention-id

mattwr18 · 2019-08-01T05:47:33Z

backend/src/middleware/xssMiddleware.js

-  })
-  return $('body').html()
-}
-
 function clean(dirty) {
  if (!dirty) {
    return dirty
  }

  // Convert embeds to a-tags


unnecessary comment now?

mattwr18 · 2019-08-01T05:51:44Z

backend/src/schema/resolvers/embeds/findProvider.spec.js

+    expect(findProvider(`https://youtu.be/qkdXAtO40Fo`)).toEqual('https://www.youtube.com/oembed')
+  })
+
+  it('matches `https://youtu.be/qkdXAtO40Fo?t=41`', () => {


was this test just to give the edge case of non-alphanumeric characters in the URL?

No, I wanted to see if youtube shareable links with a time position ?t=41 cause troubles or not.

mattwr18 · 2019-08-01T05:55:00Z

webapp/components/Editor/Editor.story.js

+]
+helpers.init({ plugins })
+
+const users = [{ id: 1, slug: 'peter' }, { id: 1, slug: 'sandra' }, { id: 1, slug: 'jane' }]


it's weird they all have the same id, no? 😛

mattwr18 · 2019-08-01T05:57:08Z

webapp/components/Editor/Editor.story.js

+      content: `
+        <p>
+          Here you can mention people like
+          <a class="mention" href="/profile/1" target="_blank" contenteditable="false">@sandra</a> and others.


missing data-mention-id, right?

mattwr18 · 2019-08-01T05:58:57Z

webapp/components/Editor/Editor.vue

@@ -183,30 +183,16 @@
 </template>

 <script>
+import defaultExtensions from './defaultExtensions.js'


Like this change

mattwr18 · 2019-08-01T06:03:32Z

webapp/components/Editor/defaultExtensions.spec.js

+  describe('`content` contains a mentioning', () => {
+    beforeEach(() => {
+      content =
+        '<p>This is a post content mentioning <a class="mention" href="/profile/f0628376-e692-4167-bdb4-d521de5a014f" target="_blank">@alicia-luettgen</a>.</p>'


data-mention-id?

is it added in toDom?

Since parseDOM is returning empty array, Mention and Hashtag never get parsed from html. It's a one-way, let's say "write-only".

By the way: I was worried I could break the hashtag implementation if hashtags are not parsed:

Tag a post with 3 hashtags

Edit the post without touching the hashtags

Are the hashtags gone now? - No, because the backend implementation searches for a[href] with a href starting with /search/hashtag/.

It's sometimes really, really confusing because all the implementations are interconnected. We have to be very careful about refactoring it.

mattwr18 · 2019-08-01T06:28:12Z

webapp/components/Editor/defaultExtensions.spec.js

+      expect(editor.getJSON()).toEqual(expected)
+    })
+  })
+})


really great that you added more tests for the Editor!!

Not testing the Editor.vue component. Only tiptap's editor. Nevertheless, I hope this tests gives us some assurance about how HTML get parsed for Editor.vue and ContentViewer.vue.

#952 (review)

Tirokk

Moin @roschaefer ,

I have tried out the embeds.
Really cool job you did!!! 🚀💫 Yuhuuhh !!!

In general it functions very well, but I have found some problems and missfunctionings. And I have additional suggestions.

General functional problems or questions

If I paste a link -> embed, the link should have target „_blank“. Otherwise I leave the editor page by a click on it.
On creation a mention and a hashtag appears as a block. If I edit the post later, it’s not a block anymore. Otherwise an embed appears always as block, and a keyboard written link always appears not as a block.
After I paste a link getting an embed the cursor is before it and not afterwards, as I would await.
If you paste a link in the line under an embed, the new link slips into the embed:

URLs which don’t work

Because of underscores I think:

http://backreaction.blogspot.com/2019/07/the-forgotten-solution-superdeterminism.html?utm_source=feedburner&utm_medium=email&utm_campaign=Feed%3A+blogspot%2Fermku+%28Backreaction%29
https://de.wikipedia.org/wiki/Yin_und_Yang

Suggestions:

If there is no preview image in the post, just always take the image from the metadata. Even with YouTube videos etc.. The images are kind of always available, see examples:

@Tirokk

* Add target="_blank" (on embeds only!) * When pasting a link, the cursor position is moved after the paste * Can't reproduce a link slipping into the embed in front of it @Tirokk it is an unpleasant side efffect that mentions + hastags appear differently on Edit+View. That's because they don't get parsed from HTML, it's a one way, they are write only. So, when viewing content, hashtags and mentions appear as plain links. I don't think I can do anything about it. Regarding some links not being embedded: Only those links that have an oembed provider in this file: https://github.com/Human-Connection/Human-Connection/blob/f44d0f1f96f97a1001b5edcc32426b5da7ff6074/backend/src/schema/resolvers/embeds/providers.json ...will be embedded. Your example `http://backreaction.blogspot.com` and `https://de.wikipedia.org/wiki/Yin_und_Yang` have no embed provider and won't be embedded. We would have to add oembed providers to this list if we wanted to embed those links, too.

Tirokk

Moin @roschaefer !

Can't reproduce a link slipping into the embed in front of it: This error vanished now, probably because of the corrected curser position after pasting an link->embed!?

You misunderstood. Pasting in the links:

http://backreaction.blogspot.com/2019/07/the-forgotten-solution-superdeterminism.html?utm_source=feedburner&utm_medium=email&utm_campaign=Feed%3A+blogspot%2Fermku+%28Backreaction%29
https://de.wikipedia.org/wiki/Yin_und_Yang

… just don’t work correct, because of a problem with underscores I think. Has nothing todo with embeds.

My suggestion to use the image url from the metadata as the posts teaser image, if there is none, was just in general. No matter if you turn the link into an embed or not. @mattwr18 thinks that this would be a great idea as well.

If one pastes just a link->embed in a post, the save button is disabled. That comes because of the content sanitisation on HTML, before testing the content length. The sanitisation I find very smart and important. Can we just check additionally in file webapp/components/ContributionForm/ContributionForm.vue in function manageContent for setting disabledByContent, if theres an embed in the content?

Tirokk · 2019-08-05T12:09:06Z

@roschaefer your new regex seems not to work correctly!?
I can show you …

Apparently the default pasteRules of tiptap interfere with the pasteRules of a Link (in our case an Embed node). Consider this example: https://de.wikipedia.org/wiki/Yin_und_Yang Depending on some random conditions, tiptap might parse the `_und_` to be italic because it's wrapped with underscores (markdown syntax). The result is: https://de.wikipedia.org/wiki/Yin # link _und_ # italic Yang # plain text So let's remove the default pasteRules of `Bold`, `Strike` and `Italic` marks respectively to prefer our Embeds. Who is copy+pasting from one tiptap editor to another tiptap editor anyways?

Tirokk

Yeah @roschaefer , now you got it running correctly.

Astonishing coding again my dear, a big chunk of code and huge step forward to our release !!! 🚀🚀💫
Thank you for all your effort you put again into our project.

You are one of our heroes … 😍

appinteractive and others added 7 commits June 24, 2019 21:03

First attempt of using storybook for component dev

fb9a860

Added actions and accessibility addons

2b80c12

added one more post story

cc64f43

Fixed small issues

e8f1c04

Fixed no-ssr in storybook

e4279c3

Added basic embed link rendering

6d852b5

Add href to link

30c5558

roschaefer added 3 commits July 22, 2019 13:27

Merge remote-tracking branch 'origin/master' into storybook

455eac1

Fix import path

f71e481

roschaefer force-pushed the storybook branch from 01617ed to 9674062 Compare July 29, 2019 12:14

roschaefer changed the title ~~[WIP] Storybook~~ Storybook Jul 29, 2019

roschaefer requested review from appinteractive, ulfgebhardt, alina-beck, ogerly and Tirokk July 29, 2019 12:25

roschaefer force-pushed the storybook branch from 4cc8840 to c4da3d8 Compare July 31, 2019 15:00

roschaefer added 2 commits August 1, 2019 00:08

Merge remote-tracking branch 'origin/master' into storybook

34f86c9

roschaefer force-pushed the storybook branch from c0bb19b to 72edf78 Compare July 31, 2019 22:33

mattwr18 commented Aug 1, 2019

View reviewed changes

roschaefer added 2 commits August 1, 2019 12:20

Followed @mattwr18's suggestions here:

c9e6d18

#952 (review)

Merge remote-tracking branch 'origin/master' into storybook

b6a5946

roschaefer force-pushed the storybook branch from 4982b60 to 4324333 Compare August 2, 2019 12:17

Meld commits from master

f1b28c8

roschaefer force-pushed the storybook branch from 4324333 to a6aa974 Compare August 2, 2019 12:27

Merge remote-tracking branch 'origin/master' into storybook

a6aa974

Tirokk requested changes Aug 2, 2019

View reviewed changes

roschaefer force-pushed the storybook branch 2 times, most recently from 3b93fbe to b797ce4 Compare August 2, 2019 21:16

Tirokk requested changes Aug 5, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into storybook

1aeb93f

roschaefer force-pushed the storybook branch from 24d2125 to 8093fec Compare August 5, 2019 11:24

Update URL regex taking e.g. "?" into account

8093fec

ogerly approved these changes Aug 5, 2019

View reviewed changes

roschaefer requested a review from Tirokk August 5, 2019 15:48

roschaefer mentioned this pull request Aug 5, 2019

Link's pasteRules compete with Italic's pasteRules ueberdosis/tiptap#421

Closed

Tirokk approved these changes Aug 6, 2019

View reviewed changes

Tirokk merged commit e8d5bed into master Aug 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storybook #952

Storybook #952

mattwr18 commented Jul 2, 2019 •

edited by roschaefer

codecov bot commented Jul 2, 2019 •

edited

mattwr18 commented Jul 3, 2019

mattwr18 left a comment

mattwr18 Aug 1, 2019

roschaefer Aug 1, 2019

mattwr18 Aug 1, 2019

roschaefer Aug 1, 2019

mattwr18 Aug 1, 2019

mattwr18 Aug 1, 2019

roschaefer Aug 1, 2019

mattwr18 Aug 1, 2019

mattwr18 Aug 1, 2019

mattwr18 Aug 1, 2019

mattwr18 Aug 1, 2019

mattwr18 Aug 1, 2019

roschaefer Aug 1, 2019

roschaefer Aug 1, 2019

mattwr18 Aug 1, 2019

roschaefer Aug 1, 2019

Tirokk left a comment

Tirokk left a comment

Tirokk commented Aug 5, 2019

Tirokk left a comment

Storybook #952

Storybook #952

Conversation

mattwr18 commented Jul 2, 2019 • edited by roschaefer

🍰 Pullrequest

codecov bot commented Jul 2, 2019 • edited

Codecov Report

mattwr18 commented Jul 3, 2019

mattwr18 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

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

Tirokk left a comment

Choose a reason for hiding this comment

General functional problems or questions

URLs which don’t work

Suggestions:

Tirokk left a comment

Choose a reason for hiding this comment

Tirokk commented Aug 5, 2019

Tirokk left a comment

Choose a reason for hiding this comment

mattwr18 commented Jul 2, 2019 •

edited by roschaefer

codecov bot commented Jul 2, 2019 •

edited