feat: support <details> tag for GithubHtmlView (#528) by whitedogg13 · Pull Request #819 · gitpoint/git-point

whitedogg13 · 2018-10-02T16:23:35Z

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

Screenshots

Before (not support details tag)

After 1 (detail not expanded)

After 2 (detail expanded)

Description

Support <details> tag in GithubHtmlView, as mentioned in #528 .

First try to find <summary> tag inside <details>, then render a toggle-able view if found, otherwise do a rollback rendering. Also try to prettify the text in <summary> if we got only one text child inside it.

Thanks!

coveralls · 2018-10-02T16:27:04Z

Coverage increased (+1.8%) to 44.189% when pulling 7cfed74 on whitedogg13:html-details-support into 027dfc5 on gitpoint:master.

lex111 · 2018-10-02T23:14:56Z

@whitedogg13 it's awesome! 👏

What do you think can write a small test for this component?

whitedogg13 · 2018-10-03T01:59:12Z

@lex111 Sure, love to do so. I will update it later.

whitedogg13 · 2018-10-03T15:25:00Z

Hi @lex111 , I have just added test cases to test this PR. Please feel free to feedback!

BTW, in this case I use react-test-renderer rather then enzyme, which is different from the convention in this project, and I'd like to briefly explain why:

In GithubHtmlView we depends on 3rd party react-native-htmlview to perform html rendering. However this library only renders a skeleton view in first render (it triggers state change to render actual content in componentDidMount...), which makes enzyme's shallow or render won't be able to test. On the other hand, react-test-render can capture this case.

Thanks!

lex111

This is good for me, but I would like to hear the opinions of other members, so for now I will not merge PR. Thanks!

chinesedfan

@whitedogg13 Great attempt. Maybe you can check #609 to get some inspirations. For example,

enhance toggle-view.component.js to replace WithToggle
handle the summary tag
add different edge cases to tests

whitedogg13 · 2018-10-04T15:37:48Z

Hi @chinesedfan , thanks for the review!

I made some enhancement over your first and third suggestions:

enhance toggle-view.component.js to replace WithToggle
add different edge cases to tests

It's always good to test and reuse code.

However, about the handling for <summary/> tag, I have checked #609, some points listed:

it adds a extra <hidden/> tag in order to parse <summary/>, since <hidden/> is not a valid HTML tag, so more or less like a hack to me.
it renders <summary/> with <View>{defaultRenderer(node.children, parent)}</View>, however, this basically means we don't even need to handle this case since it simply do the default behavior.

Because of above reasons, I think current version makes more sense to me.

Thanks!

chinesedfan

@whitedogg13 I tested in iOS simulator to visit #528 and #609. It worked very well. Good job!

Forgive my little OCD to mention 2 other things.

Current test files are named as component names.
If descriptions of test cases are well-written, you can remove those inner comments. Keeping them in the same format like do blabla when xxx or should xxx if/when xx.

whitedogg13 · 2018-10-05T23:49:44Z

@chinesedfan Thanks for suggestions, I will update it later :)

whitedogg13 · 2018-10-06T16:34:10Z

Hi @chinesedfan , I have pushed the changes, thanks!

lex111 · 2018-10-16T18:50:52Z

@machour hi, can we merge this PR?

machour · 2018-10-17T11:30:38Z

Hey @lex111, sorry I've been away from computer these days. Have this PR been tested on Android? If it's working there too then you have my blessing :)

lex111 · 2018-10-19T12:27:46Z

@machour I currently have no opportunity to check, maybe you have?

whitedogg13 · 2018-10-20T23:41:19Z

Hi @lex111 I have just tested this on Nexus 5 using Genymotion Emulator, and it works correctly. For further PR, I will test it in Android as well :)

housseindjirdeh

So amazing, thank you so much for this @whitedogg13 ⭐️

housseindjirdeh · 2018-10-24T02:57:29Z

src/components/toggle-view.component.js

-          {this.props.TouchableView}
+          {this.props.renderTouchable
+            ? this.props.renderTouchable(this.state.collapsed)
+            : this.props.TouchableView}


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

f4f9d61

chinesedfan requested a review from machour October 2, 2018 16:36

lex111 requested a review from chinesedfan October 2, 2018 23:15

test: add test case for GithubHtmlView to test custom <details/> logic

c93723a

lex111 approved these changes Oct 3, 2018

View reviewed changes

chinesedfan requested changes Oct 4, 2018

View reviewed changes

whitedogg13 added 3 commits October 4, 2018 22:14

test: add more test cases against edge cases for GithubHtmlView

376517c

refactor: enhance ToggleView and utilize it in GithubHtmlView

950633e

refactor: remove extra guard against node.children

1ac9a81

chinesedfan reviewed Oct 5, 2018

View reviewed changes

chinesedfan added the markdown label Oct 6, 2018

refactor: rename test file for GithubHtmlView component

7cfed74

chinesedfan approved these changes Oct 6, 2018

View reviewed changes

lex111 approved these changes Oct 6, 2018

View reviewed changes

housseindjirdeh approved these changes Oct 24, 2018

View reviewed changes

housseindjirdeh merged commit f851b76 into gitpoint:master Oct 24, 2018

This was referenced Oct 26, 2018

feat(markdown): Add support for details tag #609

Closed

Crash when markdown contain a <details> tag #636

Closed

chinesedfan mentioned this pull request Nov 30, 2018

Specific issue page leads to crash #768

Closed

andrewda mentioned this pull request Dec 1, 2018

Add more tests! #518

Open

63 tasks

Conversation

whitedogg13 commented Oct 2, 2018

Screenshots

Description

Uh oh!

coveralls commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lex111 commented Oct 2, 2018

Uh oh!

whitedogg13 commented Oct 3, 2018

Uh oh!

whitedogg13 commented Oct 3, 2018

Uh oh!

lex111 left a comment

Choose a reason for hiding this comment

Uh oh!

chinesedfan left a comment

Choose a reason for hiding this comment

Uh oh!

whitedogg13 commented Oct 4, 2018

Uh oh!

chinesedfan left a comment

Choose a reason for hiding this comment

Uh oh!

whitedogg13 commented Oct 5, 2018

Uh oh!

whitedogg13 commented Oct 6, 2018

Uh oh!

lex111 commented Oct 16, 2018

Uh oh!

machour commented Oct 17, 2018

Uh oh!

lex111 commented Oct 19, 2018

Uh oh!

whitedogg13 commented Oct 20, 2018

Uh oh!

housseindjirdeh left a comment

Choose a reason for hiding this comment

Uh oh!

housseindjirdeh Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commented Oct 2, 2018 •

edited

Loading