Skip to content

feat: Update color converter page (HEX / RGB Converter) #44

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LeonKohli
Copy link

Hey team,
I've made some significant improvements to the Color Converter tool (HEX / RGB Converter). Instead of just handling HEX to RGB conversions, it now supports conversions between HEX, RGB, HSL, and CMYK formats.

Here's what's new:

  • Renamed to Color Converter
  • Conversions between all four color formats
  • Easier for users to select a color from the OS color picker.

I think this update will make the tool much more useful for our team and potentially save us some time when working with various color systems.
I'd appreciate it if you could take a look and let me know your thoughts. If it seems valuable to you too, I'd love to get this merged into the main branch.
Looking forward to your feedback! 🔥

Hex Color converter is now Color Converter to convert not just only Hex to RGB but even more.
@peckz peckz self-requested a review August 20, 2024 16:20
@erik-thorsell
Copy link

adding HSV to this would potentially increase interest from the luau community

@peckz
Copy link
Collaborator

peckz commented Aug 29, 2024

Thanks for creating this PR! I'm on vacation until 9th September, after that I'm going to have a look!


export type ColorValue = RGBValues | HSLValues | CMYKValues | HSVValues | string;

export const isValidHex = (hex: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a couple of unit tests for this function wouldn't hurt

return !isNaN(value) && value >= 0 && value <= 255;
};

export const convertToRGB = (hex: string): RGBValues => {
Copy link
Contributor

Choose a reason for hiding this comment

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

As well as for other conversion functions

return { r, g, b };
};

export const convertToHex = (r: string, g: string, b: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why three named parameters and not just RGBValues as input?

};

export const toCss = (rgb: RGBValues) => {
if (!rgb || typeof rgb.r === 'undefined' || typeof rgb.g === 'undefined' || typeof rgb.b === 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be possible in the first place

const valueToFixed = (value: string) => (parseInt(value) / 255).toFixed(2);

export const toIOS = (rgb: RGBValues, lang: "swift" | "c"): string => {
if (!rgb || typeof rgb.r === 'undefined' || typeof rgb.g === 'undefined' || typeof rgb.b === 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some point on unnecessary checks here

@@ -0,0 +1,222 @@
export interface RGBValues {
r: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use number here in the first place?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants