-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
[Refactor] Extend color scheme management to sequential schemes #6150
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6150 +/- ##
=======================================
Coverage 76.91% 76.91%
=======================================
Files 47 47
Lines 9362 9362
=======================================
Hits 7201 7201
Misses 2161 2161 Continue to review full report at Codecov.
|
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.
lgtm with one question about variable naming in the Registry
world.
@@ -25,7 +25,7 @@ import { nonEmpty } from '../../validators'; | |||
import vizTypes from '../../visTypes'; | |||
|
|||
import { t } from '../../../locales'; | |||
import { getScheme } from '../../../modules/ColorSchemeManager'; | |||
import getCategoricalSchemeRegistry from '../../../modules/colors/CategoricalSchemeRegistrySingleton'; |
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.
do you think this (and analogous calls in other files) should be ClassCase
since it's an instance and you still call .get()
on it?
i.e., CategoricalSchemeRegistry.get()...
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.
Currently all registries exports the getInstance
function as default.
export default getInstance;
usage as
import getXXX from 'XXXSingleton';
I will have to modify the export to
export default {
get: getInstance
};
and imports to
import { get } from 'XXXSingleton';
to make calls become XXX.get()
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.
Got it, sorry I was thinking about this a litle differently. I think it makes sense as is 👍
…he#6150) * refactor color scheme * Update data structure * Update color scheme files * wip * convert all sequential schemes * Update how color schemes are managed. Extend it for sequential schemes * extract color setup into separate file * Update imports * update imports * Add new functions to Registry * Update ColorSchemeManager to extends Registry and update unit tests * Add test for Registry * Rename ColorSchemeManager to ColorSchemeRegistry * Fix unit tests * Update API * Fix imports * Add label field * Fix reference to colors * update SequentialScheme contructor * Fix controls * rename manager to registry * Split sequential schemes into multiple files * update sequential color labels * add values and valuesAsPromise() * use .values() (cherry picked from commit b9257b2)
…he#6150) * refactor color scheme * Update data structure * Update color scheme files * wip * convert all sequential schemes * Update how color schemes are managed. Extend it for sequential schemes * extract color setup into separate file * Update imports * update imports * Add new functions to Registry * Update ColorSchemeManager to extends Registry and update unit tests * Add test for Registry * Rename ColorSchemeManager to ColorSchemeRegistry * Fix unit tests * Update API * Fix imports * Add label field * Fix reference to colors * update SequentialScheme contructor * Fix controls * rename manager to registry * Split sequential schemes into multiple files * update sequential color labels * add values and valuesAsPromise() * use .values()
Motivation
Centralize color management for ease of maintenance and customization. Also good for future migration to an independent
npm
package insuperset-ui-core
.controls.jsx
. The colors are in separate file.Changes
Create data structure
ColorScheme
and childrenCategoricalScheme
andSequentialScheme
to holdcolors
array, and its metadata (name
,label
,description
,isDiverging
) in one place. Convert the arrays of colors incolorSchemes
directory andsequential.js
to use these new data structures.Leverage
Registry
class that was introduced with the chart plugin system.ColorSchemeManager
is renamed toColorSchemeRegistry
and extends fromRegistry
.Add new function
getMap
,getMapAsPromise
,values()
,valuesAsPromise()
andclear
toRegistry
with new unit tests.Create two children singleton classes
CategoricalSchemeManager
andSequentialSchemeManager
to manage categorical and sequential scheme, respectively.Update usage and corresponding unit tests throughout the app.
Extract color setup from
common.js
intosetupColors.js
Move color-related modules under
src/modules/colors
directory and unit tests underspec/modules/colors
.Test plan
@williaster @conglei @graceguo-supercat @michellethomas