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

[whitedogg13]

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]

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

[lex111]

@whitedogg13 it's awesome! 👏

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

[whitedogg13]

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

[whitedogg13]

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]

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]

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

[whitedogg13]

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

[lex111]

@machour hi, can we merge this PR?

[machour]

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]

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

[whitedogg13]

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]

👏