8000 Keep multiple newlines in card output (Continue PR #83) by fremail · Pull Request #373 · alexa-js/alexa-app · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Keep multiple newlines in card output (Continue PR #83) #373

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 23, 2018
Merged

Keep multiple newlines in card output (Continue PR #83) #373

merged 7 commits into from
Oct 23, 2018

Conversation

fremail
Copy link
Contributor
@fremail fremail commented Oct 14, 2018

See PR #83 (2 years old hehe)

I merged the changes to current code and added tests.

kielni and others added 4 commits September 17, 2016 17:19
@kielni
keep multiple newlines in card output
1c002db 
don't replace multiple newlines with one when stripping SSML from card content
newlines are the only formatting allowed in card content, so don't remove them
@fremail
Merge branch 'multi-newline-for-cards' of git://github.com/kielni/ale…
d22ac8b 
…xa-app into continue-83
@fremail
Merge and add tests for multi line cards
25831e0
@fremail
update changelog
2e285c9
@kobim
Copy link
Contributor
kobim commented Oct 21, 2018

Hi @fremail, thanks for your contribution!

Can you please sync merge from master so the Travis CI build will pass?

Also, I have the following doubts:

  1. If the only call to cleanse is in index.js, why there's a new parameter to the function? If this is the only usage, it should be the default behavior.
  2. I think the reason behind this regex is to remove whitespaces at the end and start of lines, and your code doesn't handle that. Maybe replacing the current patten with a custom version of \s will be better? (e.g. [ \t\r\f])
  3. Any reason you omitted tests from test_ssml.js? As the real change happens in a function related to to-ssml.js, I think the tests should occur there.

I think it's time we close this 2 years old issue :)

@kobim kobim self-assigned this Oct 21, 2018
@fremail
Copy link
Contributor Author
fremail commented Oct 22, 2018 •
edited
Loading

@kobim my answers

  1. I didn't change the original changes had been made in PR keep multiple newlines in card output #83, but actually I agree with you: I don't see any reason in adding new param to the function.

  2. Oh, it's a good catch! Thank you for finding that bug in the change. I'll work to fix it.

  3. Really good question! I don't know why I did so :D

< 8000 /td>

@kobim kobim added this to the 4.2.3 milestone Oct 23, 2018
@kobim
Copy link
Contributor
kobim commented Oct 23, 2018

LGTM!
I'll merge it to the master and will it will be released along with 4.2.3 (hopefully along with #378) this weekend.

Thanks again!

@kobim kobim merged commit f718b32 into alexa-js:master Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobim kobim kobim approved these changes

Assignees

@kobim kobim

Labels
None yet
Projects
None yet
Milestone
4.2.3
Development

Successfully merging this pull request may close these issues.
3 participants
@fremail @kobim @kielni
0