-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feature - New set of color palette added #206
base: develop
Are you sure you want to change the base?
Feature - New set of color palette added #206
Conversation
✅ Deploy Preview for racingbars ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thank you @ShubhamShakyawal I prefer not to expose color palettes in the external API of the library as one of the main exports. What I suggest:
export const palettes = {
deep6,
// ...
} as const;
export type Palette = keyof typeof palettes;
colorMap: { [key: string]: string } | string[] | Palette;
Does that make sense? |
All the necessary changes are being made as you mentioned. Please consider checking it. If there is anything I can do, or if changes are required, please let me know. Palette set is used to validate in constant time in validateColorMap(). |
Preview in LiveCodesLatest commit: 396cd27
See documentations for usage instructions. |
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.
Thank you @ShubhamShakyawal
I have added some comments. You can find them in the code review.
In addition:
To load the selected color palette, we need to replace this with:
const colorMap = Array.isArray(options.colorMap)
? [...options.colorMap].map(String)
: typeof options.colorMap === 'object'
? { ...options.colorMap }
: options.colorMap && options.colorMap in palettes
? palettes[options.colorMap]
: state.colorMap;
Also, please run npm run fix
before committing to fix failing formatting check.
Thank you.
@hatemhosny I have made changes to what you have mentioned. On
After
|
Please only add the files you have changed.
This is only a warning. It should not cause the check to fail. This should go when we upgrade dependencies. |
The changes you mentioned have been made, but the build was not successful. Please look into this matter. |
Thank you @ShubhamShakyawal , I will have a look. You may also want to include palettes from this project: https://github.com/pratiman-91/colormaps |
I have fixed linting and formatting errors in these commits:
We can just use a few of these palettes, not all of them. |
Quality Gate passedIssues Measures |
I have added some more palettes. |
What kind of change does this PR introduce?
Feature
What is the current behavior?
Currently, there is no color palette available for colorMap in Options.
What is the new behavior (if this is a feature change)?
Yes, It is a feature where users can choose amongst the 12 available color palettes.
Issue related to this PR
#198
The changes have been discussed in an issue and the approach has been approved.
issue link: provide a set of color palettes #198
Other information:
This PR creates a new file named palette.ts in src/lib/.
Users can choose amongst the 12 available color palette options.
How to use.
List of available color palettes.