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

faker.internet.color -> faker.color.? #1959

Open
matthewmayer opened this issue Mar 21, 2023 · 10 comments
Open

faker.internet.color -> faker.color.? #1959

matthewmayer opened this issue Mar 21, 2023 · 10 comments
Assignees
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: color Something is referring to the color module m: internet Something is referring to the internet module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug s: needs decision Needs team/maintainer decision
Milestone

Comments

@matthewmayer
Copy link
Contributor

faker.internet.color is a bit of an oddity in the faker.internet module which "Generates a random css hex color code in aesthetically pleasing color palette."

Deprecate and move to... faker.color.internet? faker.color.rgbFromBase?

@matthewmayer matthewmayer added s: needs decision Needs team/maintainer decision c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: color Something is referring to the color module m: internet Something is referring to the internet module labels Mar 21, 2023
@matthewmayer matthewmayer moved this to Todo in Faker Roadmap Mar 21, 2023
@matthewmayer matthewmayer self-assigned this Mar 21, 2023
@Shinigami92
Copy link
Member

Just a random thought for "internet css colors"
There is a finite set of all browser supported named css colors: https://www.w3schools.com/cssref/css_colors.php

@matthewmayer
Copy link
Contributor Author

without reading the code, i would never have guessed the current implementation.

@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Mar 21, 2023
@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Mar 22, 2023

TBH, I don't see a problem here. I dont think we need to change anything.

The ColorModule obviously has a lot of different color-related functions.
The InternetModule obviously has many different functions related to the internet.

If you now think about a color in the internet (from a dev perspective) - CSS is the first thing that comes to my mind.
I may be biased since I am a frontend dev.

@xDivisionByZerox
Copy link
Member

without reading the code, i would never have guessed the current implementation.

Good thing we have documentation, so you don't need to read the source code :)

@ST-DDT
Copy link
Member

ST-DDT commented Mar 23, 2023

Team Decision

We will try to merge the color function with color.rgb as an option.
In the long run we want to deprecate the internet.color method.

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Mar 23, 2023
@matthewmayer
Copy link
Contributor Author

this seems logical, it could be an option for faker.color.rgb to make "nice" colors.

Also TBH if you want "nice" colors nowadays it's way easier to not use RGB but another color space like HSB or HSL, because you can fix the saturation and brightness/lightness and just change the hue, and you get nice matching colors.

@Shinigami92
Copy link
Member

this seems logical, it could be an option for faker.color.rgb to make "nice" colors.

Also TBH if you want "nice" colors nowadays it's way easier to not use RGB but another color space like HSB or HSL, because you can fix the saturation and brightness/lightness and just change the hue, and you get nice matching colors.

So you mean we can just deprecate the internet.color at all and just suggest to switch to the color module and use a fitting method for the use case?

@matthewmayer
Copy link
Contributor Author

Not quite because the faker.color.* etc don't have a way to supply a base color. So I guess that would be needed first.

@matthewmayer
Copy link
Contributor Author

For reference, some example colors for faker.internet.color (with and without a base color) vs faker.color.rgb:

image

https://stackblitz.com/edit/faker-js-demo-861y3v

whether its "aethetically pleasing" is a matter of opinion :)

@ST-DDT
Copy link
Member

ST-DDT commented Dec 26, 2024

Should we port the base color feature over, or should we just deprecate the old method?

Usage:



If we do port the feature over, we should probably define it as:

  • options: { base: string, ...} (string -> '#color')
  • or options: { base: string | { red, green, blue}, ...}

instead of the current options: { redBase, greenBase, blueBase }

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: color Something is referring to the color module m: internet Something is referring to the internet module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug s: needs decision Needs team/maintainer decision
Projects
No open projects
Status: Todo
Development

No branches or pull requests

4 participants