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

#116 - Add nl2br setting to text property #135

Merged
merged 1 commit into from Jan 12, 2016

Conversation

Projects
None yet
3 participants
@nlemoine
Contributor

nlemoine commented Jan 12, 2016

Ok, finally got it working. Sorry, I'm not familiar with unit testing and struggled a little bit with it.

Am I wrong or is the test case for text property isn't testing settings at all (allow_html)? I also noticed another notice regarding the papi_nl2br but I will address you an specific issue on this.

I also set the default nl2br setting to true, which seems the best behaviour according to me (regular users don't understand why a line break in a textarea don't show as a line break on the front end). I don't know you feel about this. I can change it if you see things differently.

@frozzare

This comment has been minimized.

Show comment
Hide comment
@frozzare

frozzare Jan 12, 2016

Member

No problem, looks like the test you did works.

Text test cases are testing allow_html, so if you select a property that has allow_html => true it will allow html.

Yeah, I think true as a default value for nl2br is good or what do you think @rasmusbe?

Member

frozzare commented Jan 12, 2016

No problem, looks like the test you did works.

Text test cases are testing allow_html, so if you select a property that has allow_html => true it will allow html.

Yeah, I think true as a default value for nl2br is good or what do you think @rasmusbe?

@rasmusbe

This comment has been minimized.

Show comment
Hide comment
@rasmusbe

rasmusbe Jan 12, 2016

Contributor

I agree that default should be nl2br, but it's a breaking change so be sure to write it in the changelog for papi 3.

Contributor

rasmusbe commented Jan 12, 2016

I agree that default should be nl2br, but it's a breaking change so be sure to write it in the changelog for papi 3.

frozzare added a commit that referenced this pull request Jan 12, 2016

Merge pull request #135 from nlemoine/feature-nl2br
#116 - Add nl2br setting to text property

@frozzare frozzare merged commit 75a1c4c into wp-papi:master Jan 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment