feat(markdown): Add support for details tag by ArvinH · Pull Request #609 · gitpoint/git-point

ArvinH · 2017-10-30T13:32:32Z

feat(markdown): Add support for details tag

Question	Response
Version?	v1.4.0
Devices tested?	iPhone 6
Bug fix?	no
New feature?	yes
Includes tests?	no
All Tests pass?	yes
Related ticket?	#528

Screenshots

Before	After

More test cases in here:

Description

Add support for github <details> markdown tag.
Modify ToggleView component for supporting state-based render

coveralls · 2017-10-30T13:44:42Z

Coverage decreased (-0.6%) to 38.809% when pulling db4390e on ArvinH:feat-markupd-details into 753f60c on gitpoint:master.

coveralls · 2017-10-30T13:47:20Z

Coverage decreased (-0.6%) to 38.809% when pulling db4390e on ArvinH:feat-markupd-details into 753f60c on gitpoint:master.

machour

Wow, that's an awesome start @ArvinH!

I still need to pull down this PR and see if we can simplify and bullet proof childrendComponent construction, but this seems really promissing.

Left you some comments for things that needs fixing.

Markdown rendering is a quiet complicated stuff to work on. While it may work for you on the exact markdown sample you test on, it can break as soon as someone add a space in their markdown 😢

Could you try setting up some more markdown cases? Try using an image/list/code section as summary and see how your PR handles them.

Again, can't thank you enough for diving into this component! 👏 👏

machour · 2017-10-30T15:21:48Z

src/components/toggle-view.component.js

    this.setState({ collapsed: !this.state.collapsed });
  }

+  renderTochableView() {


Oops, typo.

machour · 2017-10-30T15:27:59Z

src/components/github-htmlview.component.js

+            if (child.type === 'tag' && child.name === 'summary') {
+              const summaryChild = child.children[0] || {};
+
+              summaryText = summaryChild.data || 'details';


The default is redundant with summaryText declaration.

Also, summaryChild may need additional treatment (if it's an image for example). It needs to be rendered()

I guess I was too hasty, didn't consider other cases of summaryChild. Thank you so much for your patient 😄

No problems, I did make the same mistake quiet a few times before getting on pace :)
I'm available in gitter today if you need some help!

machour · 2017-10-30T15:30:37Z

src/components/github-htmlview.component.js

+
+          detailsChildren.forEach((child, childIdx) => {
+            if (child.type === 'tag' && child.name === 'summary') {
+              const summaryChild = child.children[0] || {};


from experience, I know that using .children[x] is bad. Imagine if the first child is a space for example.
You may want to use the onlyTagsChildren here.

Yeah, I'm not familiar with markdown, I basically keep switching to http://astexplorer.net/ to check the AST parsing result :p
I didn't notice we may have space in first children 😭

Should use onlyTagsChildren here. 👍

machour · 2017-10-30T15:34:39Z

src/components/github-htmlview.component.js

        },
+        details: (node, index, siblings, parent, defaultRenderer) => {
+          const detailsChildren = node.children || [];
+          const childrendComponent = [];


The variable name is unclear to me. What about hiddenChildren ?

machour · 2017-10-30T15:36:38Z

src/components/github-htmlview.component.js

+          return (
+            <ToggleView
+              TouchableView={collapse => {
+                const prefixNotation = collapse ? '▸' : '▾';


Those little babies (▸ / ▾) need some style to be bigger.
prefix could be a simpler & better variable name

machour · 2017-10-30T15:37:31Z

src/components/toggle-view.component.js

+    const { TouchableView } = this.props;
+
+    if (TouchableView instanceof Function) {
+      return TouchableView(this.state.collapsed);


Really love what you did here ❤️

ArvinH · 2017-10-31T14:36:07Z

Fot test:

`<code>` in summary:

console.log('test');

try something for summary

`<image>` in summary:

try something for summary

`<li>` in summary:

test 1
test2

try something for summary

machour · 2017-10-31T14:39:57Z

Paladin level 95:

Summary as detail 😆

nested summary details

top summary details

machour · 2017-11-06T11:01:41Z

Hey @ArvinH, just checking on you to see if you're stuck on some technical difficulty for this PR.
No rush if you're busy with something else!

ArvinH · 2017-11-06T13:31:32Z

Yeah, I'm kind of busy these days, I'll come back this week!
And I actually had some progress on this one but still looking for a better way to organize my code.

refactor(markdown): details tag support with arrow notation refactor(markdown): details tag support with image, nested summary feat(markdown): support code in summary refactor(markdown): defensive code & remove conosle refactor(markdown): neater way to construct details tag refactor(markdown): adjust prefix style

coveralls · 2017-11-11T09:50:49Z

Coverage decreased (-0.01%) to 39.849% when pulling 129e76e on ArvinH:feat-markupd-details into 05c386e on gitpoint:master.

machour · 2017-11-11T12:09:06Z

src/components/github-htmlview.component.js

          return undefined;
        },
+        details: (node, index, siblings, parent, defaultRenderer) => {
+          const tags = onlyTagsChildren(node);


Nice nice nice 🎉
Let's check that there are indeed two tags and that they are indeed summary and hidden.
Define them here with consts for later use

hmm, I'm thinking how we should handle the case of no summary or no hidden tags in details tag.
May need a default summary and empty hidden for this case.

This shouldn't really happen I think, but I'm ok with your suggestion
The goal is to avoid a crash on [1]

What [1] means?

coveralls · 2017-11-11T13:19:57Z

Coverage decreased (-0.3%) to 39.599% when pulling 567980e on ArvinH:feat-markupd-details into 05c386e on gitpoint:master.

coveralls · 2017-11-11T16:24:40Z

Coverage decreased (-0.3%) to 39.549% when pulling 35b7241 on ArvinH:feat-markupd-details into 05c386e on gitpoint:master.

machour · 2017-11-11T16:58:26Z

src/components/github-htmlview.component.js

+        details: (node, index, siblings, parent, defaultRenderer) => {
+          const tags = onlyTagsChildren(node);
+          const firstTag = tags[0] || {};
+          const secondTag = tags[1] || {};


@vitalkanev I was referring to these calls that were in the middle of the jsx

All remarks have been handled

machour · 2017-11-11T17:24:35Z

@ArvinH thank you so much for your work on this PR homie 🥇
I'll give it a thorough review on Android and iOS as soon as I can

machour

@ArvinH sorry for not getting back to you earlier, I was too busy :/

I tested this PR against #528 and it crashes with "Views nested in <Text> must have a height and width".

This is due to the hidden section containing a heading, which produces a <View>

And it made me realize that as we can include any kind of element in the hidden section, we will have this problem with blockquote, hr, and maybe some other tags.

I know this is outside the scope of this PR, and I'm unsure how to address those issues.

Maybe we need to completely change the way we produce those tags, measure them before rendering then set height/width everywhere to be safe.

@housseindjirdeh This was originally supposed to be part of 1.4 (to fix #636) but it turns out to be rather complicated and revealing of deep problems with our current markdown rendering. Are you okay to drop this feat/fix from 1.4?

machour · 2017-11-23T12:51:38Z

src/components/github-htmlview.component.js

+          );
+        },
+        summary: (node, index, siblings, parent, defaultRenderer) => {
+          return <View>{defaultRenderer(node.children, parent)}</View>;


The <View> wrapper is not needed here

ArvinH · 2017-12-07T03:55:16Z

Sorry I was too busy these days, didn't have the chance to come back here.
Anyway, I'll have some spare time next week and I guess I'll have time to refactor this and discuss with you on gitter 😎

machour · 2017-12-07T11:15:12Z

@ArvinH no worries, we all get busy from time to time :)

I'm currently thinking of completely changing one's ground on markdown:

We currently rely on Github to transform the markdown, and we're dealing with HTML.
This is somehow a bad idea, because if Github changes anything at some point, the app can break.
We're also somehow crippled by react-native-htmlview.

I'd like to get back to real markdown parsing, and extract this component into its own repo/lib (like gitpoint/react-native-markdown-view).

Features:

Use markdown-it for parsing
Ditch react-native-htmlview in favor of our own renderer, built on top of htmlparser2
Support Github markdown in priority, but also other flavours (bitbucket, gitlab, etc..)
Be GitPoint independant
Tests, Tests, Tests, Tests

I've been experimenting with this on my side, but nothing really working for now. I'd love it if you could join next week to dive with me in this :)

(cc @gitpoint/maintainers for feedback)

chinesedfan · 2018-10-26T15:31:55Z

As #819 was merged, I will close this one. Thanks for @ArvinH's efforts all the same!

ArvinH force-pushed the feat-markupd-details branch from 6b0329b to db4390e Compare October 30, 2017 13:34

machour previously requested changes Oct 30, 2017

View reviewed changes

machour self-assigned this Oct 30, 2017

machour added markdown 🏗️ work in progress labels Oct 30, 2017

machour mentioned this pull request Nov 7, 2017

Crash when markdown contain a <details> tag #636

Closed

ArvinH force-pushed the feat-markupd-details branch from db4390e to 129e76e Compare November 11, 2017 09:42

machour reviewed Nov 11, 2017

View reviewed changes

refactor(markdown): add more defensive code

567980e

refactor(markdown): check tags name and add default summary

35b7241

machour reviewed Nov 11, 2017

View reviewed changes

machour added ready for review and removed 🏗️ work in progress labels Nov 11, 2017

machour requested changes Nov 23, 2017

View reviewed changes

machour mentioned this pull request Dec 10, 2017

fix(crash): Fix potential crash on undefined parent in <code> #662

Merged

whitedogg13 mentioned this pull request Oct 4, 2018

feat: support <details> tag for GithubHtmlView (#528) #819

Merged

chinesedfan closed this Oct 26, 2018

Conversation

ArvinH commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Description

Uh oh!

coveralls commented Oct 30, 2017

Uh oh!

coveralls commented Oct 30, 2017

Uh oh!

machour left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArvinH commented Oct 31, 2017

<code> in summary:

<image> in summary:

<li> in summary:

Uh oh!

machour commented Oct 31, 2017

Uh oh!

machour commented Nov 6, 2017

Uh oh!

ArvinH commented Nov 6, 2017

Uh oh!

coveralls commented Nov 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 11, 2017

Uh oh!

coveralls commented Nov 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

machour commented Nov 11, 2017

Uh oh!

machour left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArvinH commented Dec 7, 2017

Uh oh!

machour commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinesedfan commented Oct 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ArvinH commented Oct 30, 2017 •

edited

Loading

`<code>` in summary:

`<image>` in summary:

`<li>` in summary:

machour commented Dec 7, 2017 •

edited

Loading