Add option for whether ambiguous width characters should be fullwidth or halfwidth #5
Conversation
|
It should be an option in an optional options object as the second argument, not a separate method. ( It's also missing docs. Reference on how it should be implemented and documented. |
sindresorhus
changed the title
| @@ -1,35 +1,226 @@ | |||
| 'use strict'; | |||
| const stripAnsi = require('strip-ansi'); | |||
| const isFullwidthCodePoint = require('is-fullwidth-code-point'); | |||
| const codePointAt = require('code-point-at'); | |||
There was a problem hiding this comment.
Just use String#codePointAt().
| }; | ||
|
|
||
| function ambiguousChar(code) { | ||
| if ((code === 0x00A1) || |
There was a problem hiding this comment.
What's the source of all these code point ranges?
There was a problem hiding this comment.
University of Cambridge:
https://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
| @@ -1,22 +1,24 @@ | |||
| import test from 'ava'; | |||
| import m from './'; | |||
| import stringWidth from './'; | |||
|
Thanks for the code review @sindresorhus. I refactored the code and I assume a default ambiguous char width of 2 columns. Please let me know if you agree. I added a new section to the documentation - API. |
| module.exports = str => { | ||
| module.exports = (str, opts) => { | ||
| opts = Object.assign({ | ||
| ambiguousCharWidth: 2 // default |
There was a problem hiding this comment.
The default comment is moot.
| @@ -24,6 +30,12 @@ module.exports = str => { | |||
| i++; | |||
| } | |||
|
|
|||
| // could be halfwidth or fullwidth | |||
| @@ -33,3 +45,72 @@ module.exports = str => { | |||
|
|
|||
| return width; | |||
| }; | |||
|
|
|||
| function ambiguousChar(code) { | |||
There was a problem hiding this comment.
ambiguousChar => isAmbiguousChar
|
|
||
| Type: `string` | ||
|
|
||
| Text |
There was a problem hiding this comment.
Not really useful. Better to just drop it.
| @@ -29,6 +29,24 @@ stringWidth('a'); | |||
| //=> 1 | |||
| ``` | |||
|
|
|||
| ## API | |||
|
|
|||
| ### stringWidth(str, [options]) | |||
|
|
||
| ### stringWidth(str, [options]) | ||
|
|
||
| #### str |
|
|
||
| ##### ambiguousCharWidth | ||
|
|
||
| Type: `int`<br> |
There was a problem hiding this comment.
int is not a type in JS. Should be number.
| ##### ambiguousCharWidth | ||
|
|
||
| Type: `int`<br> | ||
| Values: 1 (halfwidth) or 2 (fullwidth) |
There was a problem hiding this comment.
Actually, since there are only two possible scenarios, I think we should just make it a boolean and call it: ambiguousCharsAreFullwidth or something.
|
Ugh, sorry @joelpinheiro. I did a review 16 days ago, but forgot to click the "Submit review" button... |
|
Hey, @joelpinheiro, would you be willing to finish this up? :) |
Fixes #1.
Added a set of new tests to verify the correct behaviour of this feature.