-
Notifications
You must be signed in to change notification settings - Fork 138
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
[Feature] vCard phone-number escaping #517
[Feature] vCard phone-number escaping #517
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes requested, sorry for the delay!
lib/ical/design.js
Outdated
}, | ||
"phone-number":{ | ||
fromICAL: function(aValue) { | ||
return fmap(Array.from(aValue), c => c==='\\' ? undefined : c ).join(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you run the linters? I suspect this might fail due to missing spaces around operators.
As a bonus I'd also appreciate some performance testing to see if the conversion to and from an array is faster or slower than using indexOf and friends. I seem to recall there were some perf issues with large data sets in the past on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more linter errors, see test run
f1b2139
to
d6b2c41
Compare
d6b2c41
to
9115c5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adapted the changes on top of th es6 patches, either I did something wrong or there is a bug. @workgroupengineering can you double check?
9115c5d
to
95735e5
Compare
I tried it on my machine it seems to work. |
Pull Request Test Coverage Report for Build 3264037023
💛 - Coveralls |
Pull Request Test Coverage Report for Build 3264037023
💛 - Coveralls |
rfc https://datatracker.ietf.org/doc/html/rfc6350#section-3.4
fixes nextcloud/contacts#1855
fixes nextcloud/contacts#1297