Disable hex4 and hex8 #461
Conversation
elvisvoer
force-pushed
the
disablehex8
branch
from
1963efd to
01e9db9
Compare
src/helpers/spec.js
Outdated
| @@ -92,15 +92,15 @@ describe('helpers/color', () => { | |||
| }) | |||
|
|
|||
| describe('isValidHex', () => { | |||
| test('allows strings of length 3, 6, or 8', () => { | |||
| test('allows strings of length 3 or 6', () => { | |||
| expect(color.isValidHex('f')).toBeFalsy() | |||
| expect(color.isValidHex('ff')).toBeFalsy() | |||
| expect(color.isValidHex('fff')).toBeTruthy() | |||
| // expect(color.isValidHex('ffff')).toBeFalsy() | |||
There was a problem hiding this comment.
Hey, you mind commenting this back in so that we have a guard for hex4 as well?
elvisvoer
force-pushed
the
disablehex8
branch
from
01e9db9 to
fbc5e63
Compare
|
@casesandberg great catch. I realised I was only taking colors with leading hash even though the test I modified was without. Fixed both implementation and uncommented assertion. |
|
Wonderful @elvisvoer, thanks for your work on this! |
|
I've invited you to join the repo as a collaborator – no pressure to accept! Feel free to reach out with any questions. |
|
@casesandberg thanks for merging and for the invitation. I will reach out to you with questions for sure. |
|
This is live in |
| @@ -46,7 +46,9 @@ export default { | |||
| }, | |||
|
|
|||
| isValidHex(hex) { | |||
| return tinycolor(hex).isValid() | |||
| // disable hex4 and hex8 | |||
| const lh = (String(hex).charAt(0) === '#') ? 1 : 0 | |||
There was a problem hiding this comment.
Minor point, but I'm curious: why isn't this hex[0] == '#'? Since length is checked, hex is expected to be a string.
Another alternative:
const len = hex.startsWith('#') ? hex.length - 1 : hex.length
return (len == 3 || len == 6) && tinycolor(hex).isValid()By checking that length is 3 or 6, instead of NOT 4 or >6 (per the spec of this function), it won't matter if tinycolor later allows other lengths.
Can this be fixed in version 3.0.0 as well? I would like UMD bundle with this fix, and version 3 currently supports UMD. Otherwise, please make UMD bundle for version 2 |
Closes #432