Pass threshold as a parameters

[franciscohanna92]

Hello there!

Where is this threshold value coming from? I mean, why 130?

font-color-contrast/font-color-contrast.js

Line 38 in 37f12db

if (contrast > 130) {

Would you accept a PR to receive that value as an optional parameter?

[russoedu]

Hi Francisco, of course, I would accept a pull request. I'm refactoring the whole code in TS, but I will for sure include your PR. I found this contrast algorithm somewhere (I honestly don't remember where), so, no clue where this "130" comes from. Best,

…

---- Eduardo *Russo*

On Tue, 26 Oct 2021 at 01:03, Francisco Hanna ***@***.***> wrote: Hello there! Where is this threshold value coming from? https://github.com/russoedu/font-color-contrast/blob/37f12db3d8b1a8a01d391d2a26d34296903be56f/font-color-contrast.js#L38 Would you accept a PR to receive that value as an optional parameter? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADTIUXDDWAZ5JWX23JASRLUIXV2HANCNFSM5GWO3IYQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[franciscohanna92]

After some digging around the web, I think I found the original source for this algorithm.

It's important to notice that the author recommends "a more accurate" set of value to calculates the contrast (or "perceived brightness")

brightness = sqrt( .299 R2 + .587 G2 + .114 B2 )

Should I include these changes (new values for calculation and link to the original source in README.md) in the PR?

[russoedu]

Go for it!

On Tue, 26 Oct 2021 at 13:23, Francisco Hanna ***@***.***> wrote: After some digging around the web, I think I found the original source for this algorithm <http://alienryderflex.com/hsp.html>. It's important to notice that the author recommends *"a more accurate"* set of value to calculates the contrast (or *"perceived brightness"*) brightness = sqrt( .299 R2 + .587 G2 + .114 B2 ) Should I include these changes (new values for calculation and link to the original source in README.md) in the PR? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADTIUXZ7S3PZMU36APNHEDUI2MVRANCNFSM5GWO3IYQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Eduardo Russo +44 (0) 7435 397724

[russoedu]

Hi Francisco, I'm refactoring the whole function with TypeScript, so just wondering if you want to wait or do your changes from the TS branch. I'm also improving the function comment (actually, including, as it has none). best,

…

---- Eduardo *Russo*+44 (0) 7435 397 724

On Tue, 26 Oct 2021 at 14:04, Eduardo Russo ***@***.***> wrote: Go for it! On Tue, 26 Oct 2021 at 13:23, Francisco Hanna ***@***.***> wrote: > After some digging around the web, I think I found the original source > for this algorithm <http://alienryderflex.com/hsp.html>. > > It's important to notice that the author recommends *"a more accurate"* > set of value to calculates the contrast (or *"perceived brightness"*) > > brightness = sqrt( .299 R2 + .587 G2 + .114 B2 ) > > Should I include these changes (new values for calculation and link to > the original source in README.md) in the PR? > > — > You are receiving this because you commented. > > > Reply to this email directly, view it on GitHub > <#2 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AADTIUXZ7S3PZMU36APNHEDUI2MVRANCNFSM5GWO3IYQ> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > > -- Eduardo Russo +44 (0) 7435 397724

[franciscohanna92]

@russoedu I think the best will be to do it from the TS branch!

[russoedu]

So, I finished the refactoring and it's merged to master. I'll wait for your pull request. Best,

…

---- Eduardo *Russo*

On Thu, 28 Oct 2021 at 14:13, Francisco Hanna ***@***.***> wrote: @russoedu <https://github.com/russoedu> I think the best will be to do it from the TS branch! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADTIUSS2DJYXZ6IYXX4SWTUJFD6TANCNFSM5GWO3IYQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[franciscohanna92]

@russoedu Hello there! Getting back at this after some busy days.

I see as kind of difficult (or impossible) to pass yet another optional param as the threshold. For example, if I want to use the hex number overload and use a threshold value as a second parameter, calling fontColorContrast(0XF3DC56, 0.15) will result in 0.15 being interpreted as the green value, give the function definition:

function fontColorContrast (hexColorOrRedOrArray: string | number | number[], green?: number, blue?: number, threshold?: number) { ... }

A solution could be to deprecate the overload that accepts three independent number values (there's already a number[] one which serves the same purpose). This way, each overload will accept always two params, the second one being threshold.

What do you think?

[franciscohanna92]

I've created the PR to better explain myself. Let me know what you think.

Regards.

[russoedu]

Hi Francisco, Thanks for that. I accepted the pull request in another branch and was able to adapt to keep the RGB option. I tested and all seems to be working fine. I'll be pushing to master and NPM in a bit. Best,

…

---- Eduardo *Russo*+44 (0) 7435 397 724

On Tue, 2 Nov 2021 at 22:05, Francisco Hanna ***@***.***> wrote: I've created the PR to better explain myself. Let me know what you think. Regards. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADTIUR3LLHNR3J3DZ6V3SLUKBODHANCNFSM5GWO3IYQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.