Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

Make CategoricalColorScale instance a function and remove .toFunction() #33

Merged
merged 5 commits into from
Nov 17, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Nov 15, 2018

🏆 Enhancement

Feat: Use the recently added ExtensibleFunction to make an instance of CategoricalColorScale be a function itself rather than having to call .toFunction() to convert it into a color function.

💔 Breaking Changes

BREAKING CHANGE: Remove categoricalColorScale.toFunction().
BREAKING CHANGE: The color scale no longer convert input to lowercase before finding color.

🐛 Bug Fix

Fix: Also apply cleanValue before setting color via setColor(value, color) in both color scale and color namespace levels.

@kristw kristw requested a review from a team as a code owner November 15, 2018 00:44
@kristw kristw force-pushed the kristw--color-func branch from ac794a7 to 1eb7522 Compare November 15, 2018 01:20
@kristw kristw added this to the v0.7.0 milestone Nov 15, 2018
@kristw kristw changed the title make CategoricalColorScale instance a function and remove .toFunction() Make CategoricalColorScale instance a function and remove .toFunction() Nov 15, 2018
@kristw kristw added the reviewable Ready for review label Nov 15, 2018
@codecov
Copy link

codecov bot commented Nov 15, 2018

Codecov Report

Merging #33 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #33   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          41     42    +1     
  Lines         345    344    -1     
=====================================
- Hits          345    344    -1
Impacted Files Coverage Δ
packages/superset-ui-color/src/stringifyAndTrim.js 100% <100%> (ø)
...ges/superset-ui-color/src/CategoricalColorScale.js 100% <100%> (ø) ⬆️
...superset-ui-color/src/CategoricalColorNamespace.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6156e29...1d5de68. Read the comment docs.

* Ensure value is a string
* @param {any} value
*/
export default function cleanValue(value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we possibly make this have a more specific name? clean is pretty generic, something like stringyifyAndTrim would make it more readable/clear what exactly "clean" means.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, had one suggestion for more specific func name 👌

@kristw kristw merged commit 367c841 into master Nov 17, 2018
@delete-merged-branch delete-merged-branch bot deleted the kristw--color-func branch November 17, 2018 05:24
@kristw kristw removed reviewable Ready for review labels Nov 17, 2018
@kristw kristw added #enhancement New feature or request and removed reviewable Ready for review labels Dec 6, 2018
kristw pushed a commit that referenced this pull request Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants