Skip to content
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

Support string input for number values in color methods. #243

Closed
RafalFilipek opened this issue Sep 3, 2017 · 8 comments
Closed

Support string input for number values in color methods. #243

RafalFilipek opened this issue Sep 3, 2017 · 8 comments

Comments

@RafalFilipek
Copy link

  • polished version: 1.7.0
transparentize("0.5", "#333") // "rgba(51,51,51,0.5)"
transparentize(0.5, "#333")   // "rgba(51,51,51,0.5)"
lighten("0.5", "#333")        // "#NaNNaNNaN"
lighten(0.5, "#333")          // "#b3b3b3"

What you are seeing:

As you can see if amount parametr is provided as string in lighten function you will get #NaNNaNNaN.

What you expected to see:

Accept string as a valid parameter type for float parameters or throw error.

Reproduction:

http://jsbin.com/fadevukoji/edit?js,console

Thanks!

@morajabi
Copy link
Member

morajabi commented Sep 4, 2017

@mxstbr Which behaviour do you prefer? I think we should throw an error.

@mxstbr mxstbr changed the title Inconsistent result based on parameter type (string/float) Support string input Sep 4, 2017
@mxstbr
Copy link
Member

mxstbr commented Sep 4, 2017

Couldn't we just parseFloat(input, 10) all the inputs and throw an error if it returns NaN or something? I'd rather we err on the side of accepting many inputs, /cc @bhough

@morajabi
Copy link
Member

morajabi commented Sep 4, 2017

@mxstbr Sounds good to me, but one question, why should we parse strings too? Is there any special case?

@mxstbr
Copy link
Member

mxstbr commented Sep 4, 2017

Why not? 🤷‍♂️ It's easy to implement, so why not do it? It's nicer for users for sure.

@morajabi
Copy link
Member

morajabi commented Sep 4, 2017

@mxstbr You're right, just thought if it might be bad practice and we don't want to encourage users to do it. But now, I don't think it's bad practice anymore :)

@bhough
Copy link
Contributor

bhough commented Sep 4, 2017

@mxstbr @morajabi We’ve been pretty liberal in allowing most types of inputs and having polished hande them gracefully, so I don’t see any reason not to do the same here.

At some point we should consider a composable way to do this, so we aren’t repeating the same logic in every module for transforming inputs to the proper types.

@bhough
Copy link
Contributor

bhough commented Sep 4, 2017

@mxstbr @nikgraf another point of discussion is if we want to continue to provide custom error messaging for incorrect types or start to rely more on Flow/TypeScript to cover that.

@bhough bhough changed the title Support string input Support string input for number values in color methods. Sep 4, 2017
@bhough bhough added V3 and removed discussion labels Feb 4, 2018
@bhough bhough added this to the 2.0 milestone Feb 4, 2018
bhough added a commit that referenced this issue Feb 16, 2018
All color modules now support string inputs for their amounts(percentage, amount, etc..) and
automatically convert them to ints/floats

Fix #243
bhough added a commit that referenced this issue Feb 16, 2018
All color modules now support string inputs for their amounts(percentage, amount, etc..) and
automatically convert them to ints/floats

Fix #243
@bhough
Copy link
Contributor

bhough commented Feb 16, 2018

@bhough bhough closed this as completed Feb 16, 2018
bhough added a commit that referenced this issue Mar 29, 2018
All color modules now support string inputs for their amounts(percentage, amount, etc..) and
automatically convert them to ints/floats

Fix #243
bhough added a commit that referenced this issue Jun 1, 2018
All color modules now support string inputs for their amounts(percentage, amount, etc..) and
automatically convert them to ints/floats

Fix #243
bhough added a commit that referenced this issue Aug 22, 2018
* build(typescript): Removes typescript and tsgen (#295)

Removes typescript tests and tsgen auto typing

BREAKING CHANGE: Anyone using typescript will lose typings with this release.

fix #276 #250

* build(docs): Remove doc autogeneration (#296)

Remove doc site and doc autogeneration from build to move to separate repo.

Fix #229

* feat(transitions): Same transition to multiple properties (#238)

* feat(transitions): Transitions can now apply the same values to multiple properties.

* fix(tests): Update snapshot and linting

* feat(border): Border shorthand (#297)

Border shorthand for splitting out individual border properties. Especially useful for atmoic css
libraries like Fela and Styletron.

Addresses #264

* fix(ellipsis): Harden ellipsis mixin. (#298)

Add `display: inline-flex` for browsers that support it better handle white-space issues with
`display: inline-block`

Addresses #256

* feat(fontFace): Add missing font-face properties to mixin (#299)

* feat(fontFace): Add missing font-face properties to mixin

Add font-display, font-feature-settings & font-variation-settings to fontDisplay mixin

closes #286

* chore(fontFace): Update fontFace snapshot

* fix(color): Color modules now support string amounts (#300)

All color modules now support string inputs for their amounts(percentage, amount, etc..) and
automatically convert them to ints/floats

Fix #243

* build(typescript): Removes typescript and tsgen (#295)

Removes typescript tests and tsgen auto typing

BREAKING CHANGE: Anyone using typescript will lose typings with this release.

fix #276 #250

* build(docs): Remove doc autogeneration (#296)

Remove doc site and doc autogeneration from build to move to separate repo.

Fix #229

* feat(border): Border shorthand (#297)

Border shorthand for splitting out individual border properties. Especially useful for atmoic css
libraries like Fela and Styletron.

Addresses #264

* feat(cover): Adds mixin for cover. (#304)

* build(typescript): Removes typescript and tsgen (#295)

Removes typescript tests and tsgen auto typing

BREAKING CHANGE: Anyone using typescript will lose typings with this release.

fix #276 #250

* feat(cover): Adds mixin for cover.

Adds mixin that provides css for fully covering an area, including an optional offset that acts as a
padding.

* feat(triangle): Add ability to use any units instead of only px to triangle mixin (#314)

* Fix link url to contributors' page (#307)

Just noted that link goes wrong, while have meeting with lib, fixed with a cap of tea :)

* docs(babel): add plugin (#308)

* Revert "docs(babel): add plugin (#308)" (#309)

This reverts commit 6083967.

* docs(Roadmap): Update ROADMAP.md to reflect current plan

* docs(Roadmap.md): Fixes for Roadmap.md

* feat(triangle): Add ability to use any units instead of only px to triangle mixin

closes #313

* refactor(triangle): Remove important from css declaration

BREAKING CHANGE: No longer adds important declaration

closes #313

* chore(normalize): Upgrade to normalize 8.0 (#316)

Upgrade to normalize.css 8.0 and fix bug with abbr[title] fallback.

re #315

*  feat(color): Add support for 8 CSS hexcolor values (#RRGGBBAA) (#318)

* feat(color): Add support for 8 CSS hexcolor values (#RRGGBBAA)

* chore(color): Color 8-Digit Hex Tests

Update tests for color modules to test more 8-Digit Hex cases.

* chore(Color): Properly round 8-Digit Hex

Properly round alpha values returned when processing 8-Digit Hex colors.

* chore(colors): Update 8-Digit tests

Updated 8-Digit Hex tests to ensure they match associated rgba values.

* [WIP]: feat(fluidRange): Add css-lock inspired fluidRange (#169)

* feat(fluidRange): Add css-lock inspired fluidRange

Adds fluidRange mixin that allows the user to leverage css-locks.

* feat(fluidRange): Improved fluidRange

* feat(fluidRange): fluidRange

* chore(fluidRange): more fluidRange work

* chore(fluidRange): Separate between from fluidRange

* chore(fluidRange): Update tests for fluidRange

* Update Dependencies (#319)

* chore(deps): Upgrade safe deps

* chore(deps): Upgrade warning deps

* chore(deps): Upgrade unsafe deps

Upgrade unsafe deps except for eslint 5

* chore(eslint): Upgrade eslint

Upgrade eslint and fix rollup config

* chore(types): Fix flow types (#320)

Fix issues with flow types from module changes and flow upgrade.

* chore(TypeScript): Re-enable tsgen (#321)

* chore(TypeScript): Re-enable tsgen

Re-enabled tsgen to generate TypeScript bindings.

fix #276

* build(build): Use yarn instead of npm

* test(transitions): Add error test to transitions

* Upgrade documentation.js (#322)

* docs(documentation.js): Upgrade documentation.js

Upgraded to latest release of documentation.js including fixing/updating the build.

* docs(Docs): Minor doc build fixes

Minor doc template fixes, move build away from `new Buffer`, and update toc.

* docs(docs): Cleanup docs config file

* fix(mix): Fix first param issues with mix. (#327)

Introducing a breaking change with mix that now requires the weight in order to function. The
previous version that tried to make the first param optional with a fallback broke with Babel
compilation and currying, and also did not provide the original intended API.

BREAKING CHANGE: Mix no longer has a default weight, it must always be provided.

* fix(index): Added missing modules to index.js (#328)

* feat(Flow): Define, use & document Styles type for mixins result values

* fix(Flow): Fix typo in Styles definition

* fix(Flow): Make mergeRules param type more specific to avoid type error

* fix(Typescript): Update tsgen to ^1.3 for index signature support

* chore(normalize): Update prefixed properties to camel case.

Fix #332

* chore(Flow): Fix flow errors with fluid range

* Bug Fixes (#334)

* chore(normalize): Update prefixed properties to camel case.

Fix #332

* chore(Flow): Fix flow errors with fluid range

* chore(Package): Update dependencies

* chore(docs): Docs review (#336)

* test(typeScript): Add additional TS tests (#337)

* feat(library): Deprecation warning support (#338)

* Alpha release 0 (#339)

* feat(library): Deprecation warning support

* build(package.json): Add alpha versionining to package.json

* Alpha release 1 (#340)

* feat(library): Deprecation warning support

* docs(docs): Updated Roadmap and Contributing, Travis config.

* fix(Travis): Move to single node version build

* Beta release 0 (#340) (#341)

* Alpha release 1 (#340)

* feat(library): Deprecation warning support

* docs(docs): Updated Roadmap and Contributing, Travis config.

* fix(Travis): Move to single node version build

* fix(Typescript): Fix typescript test for normalize

* fix(Multiple): Fix rounding, update dependencies, fix retinaImage

Adds rounding to several color mixins, fixes a bug with retinaImage that was causing backgroundSize
to be set as undefined, and updates deps.

* chore(package): Update deps, bump version for release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants