Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix line breaks for li #166

Merged
merged 4 commits into from Oct 16, 2017
Merged

Fix line breaks for li #166

merged 4 commits into from Oct 16, 2017

Conversation

mdimovska
Copy link

Fix line breaks for <li> elements:

  • add line break only if addLineBreaks is true

  • add line breaks only between <li> elements (do not set a line break for the last <li>, similar as it is done with paragraphs)

Milena Dimovska added 2 commits October 12, 2017 14:03
- add line break only if addLineBreaks is true
- add line breaks only between <li> elements (do not set a line break for the last <li>)
Copy link
Collaborator

@richchurcher richchurcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mdimovska and thanks for the PR 😄

Your CircleCI build is failing, probably because of linter errors. Please read the CONTRIBUTING document and it'll show you what command you need to run to fix that. You don't actually need to make any changes except the one to htmlToElement.js, so for simplicity you might want to just revert the others.

Please also add a test case that covers your change. Cheers!

@mdimovska
Copy link
Author

Hi @richchurcher, sorry for breaking the build. I fixed the linter errors. About the testing, there is already a test case, which I changed a little in order to correspond to the change I made in htmlToElement.js.
Please check the PR again. Thanks :)

@mdimovska
Copy link
Author

I have questions related to line breaks in other elements (sorry, this is not very related to this PR).

  • Is there any specific reason why there are two \n for paragraph?
  • Are these line breaks intended to behave similar as margins do (so that the separation between text looks nicer)? Or there is some other reason for this implementation?

Thanks :)

@richchurcher
Copy link
Collaborator

This is looking better. When I say add a test case, I mean one like this which specifically tests the addLineBreaks behaviour: https://github.com/jsdf/react-native-htmlview/blob/master/__tests__/HTMLView-test.js#L110-L126. So you want to make sure that it renders differently if that prop is false.

Regarding the breaks, I didn't write the original implementation but I can tell you that you can tweak margins fairly effectively if you turn 'em off: #142 (comment).

@richchurcher richchurcher merged commit 3db199e into jsdf:master Oct 16, 2017
@chapeljuice
Copy link

Any idea when this is going to be released?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants