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

rgba()->toHexa and hlsa()->toHexa not working as expected when alpha opacity #37

Open
pricop opened this issue Jun 23, 2022 · 13 comments
Open

Comments

@pricop
Copy link
Contributor

pricop commented Jun 23, 2022

When converting an rgba or hlsa color with alpha opacity value to hexa, the result is not as expected (see attachment).

image

@NikarashiHatsu
Copy link
Contributor

Did you have any repository on how to reproduce this bug?
I tried to reproduce it on my machine and somehow it just works as intended.

<?php

use OzdemirBurak\Iris\Color\Hexa;

$hex = new Hex('#b2b2b2');
$hexa = $hex->toHexa()->alpha(0.5);

?>

<div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $hexa ?>;">
    <?= $hexa ?>
</div>
<br/>

<div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $hexa->toRgba() ?>;">
    <?= $hexa->toRgba() ?>
</div>
<br/>

<div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $hexa->toHsla() ?>;">
    <?= $hexa->toHsla() ?>
</div>
<br/>

Screenshot:
Screenshot 2023-10-03 at 16 22 06

@pricop
Copy link
Contributor Author

pricop commented Oct 5, 2023

You didn't reproduce the issue as the issued was described, however, even in your example, the colors are wrong, hover over them with a color picker, the difference is very slight, but it's there.

Anyway, here's the issue more clearly, and the steps to reproduce it:

Screenshot 2023-10-06 003435

For example, the correct HSLA equivalent for RGBA should have been: hsla(0, 0%, 40%, 0.5)

Here's the code for it:

$hexa = new Rgba('rgba(102, 102, 102, 0.5)');

<div style="display: flex;">
    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $hexa ?>; margin-right: 10px;">
        <?= $hexa ?>
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $hexa ?>; margin-right: 10px;">
        <?= $hexa->toRgb() ?> RGB
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $hexa->toHex() ?>; margin-right: 10px;">
        <?= $hexa->toHex() ?> HEX
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $hexa->toHexa() ?>; margin-right: 10px;">
        <?= $hexa->toHexa() ?> HEXA
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $hexa->toHsl() ?>; margin-right: 10px;">
        <?= $hexa->toHsl() ?> HLS
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $hexa->toHsla() ?>;">
        <?= $hexa->toHsla() ?> HLSA
    </div>
</div>

@NikarashiHatsu
Copy link
Contributor

Hi, thanks for the reproducing steps, I'll try to fix the issue today.

@NikarashiHatsu
Copy link
Contributor

Hmmm, this bug is interesting.
So apparently, because RGBA is already have an alpha channel, the toHexa() or toHlsa() method did re-apply the alpha channel for the second time, resulting in even transparent color.

@NikarashiHatsu
Copy link
Contributor

NikarashiHatsu commented Oct 6, 2023

Good news, I managed to fix the issue.
I need your help to review and verify whether the new fix is working properly, cheers! 🍻

Screenshot 2023-10-06 at 11 17 53
<?php

use OzdemirBurak\Iris\Color\Rgba;

$rgba = new Rgba('rgba(102, 102, 102, 0.5)');

?>

<div style="display: flex;">
    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $rgba ?>; margin-right: 10px;">
        <?= $rgba ?>
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $rgba->toRgb() ?>; margin-right: 10px;">
        <?= $rgba->toRgb() ?>
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $rgba->toHex() ?>; margin-right: 10px;">
        <?= $rgba->toHex() ?>
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $rgba->toHexa() ?>; margin-right: 10px;">
        <?= $rgba->toHexa() ?>
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $rgba->toHsl() ?>; margin-right: 10px;">
        <?= $rgba->toHsl() ?>
    </div>

    <div style="width: 100px; height: 100px; padding: 16px; word-wrap: break-word; background-color: <?= $rgba->toHsla() ?>;">
        <?= $rgba->toHsla() ?>
    </div>
</div>

@pricop
Copy link
Contributor Author

pricop commented Oct 6, 2023

While it does appear to work better now, it still has the same issue presented in your first image, see attachment.

Untitled

@NikarashiHatsu
Copy link
Contributor

Due to the presence of the alpha channel, division should be as precise as possible if you want to create such a feat.

That is the best approach I could think of for now.

@pricop
Copy link
Contributor Author

pricop commented Oct 6, 2023

If a 1:1 conversion is not possible, then I guess it's alright (at least for my use case this is good enough). The issue can be closed if you consider so.

Thank you for the fix.

@NikarashiHatsu
Copy link
Contributor

I really need a color expert opinion on this, I will have a chat with my fellow designers to confirm whether this case is achievable or not.

Thanks for your guidance on reproducing the issue!

@pricop
Copy link
Contributor Author

pricop commented Oct 6, 2023

I think I've found out why it doesn't properly convert back to HLS.

rgb(127,127,127) to HLS should convert to hls(0, 0%, 69.8%), however iris returns hls(0, 0%, 70%) - so from what I'm seeing, adding support for decimal values would fix this.

@NikarashiHatsu
Copy link
Contributor

It might took an extra work to the PR, let me try to figure it out.
Thank you for the information.

@ozdemirburak
Copy link
Owner

ozdemirburak commented Nov 24, 2023

I've added the decimal parts, and I think it is working now.

Could you please check @pricop?

@NikarashiHatsu, I'm going to close #47 because I think there is no need to clone the colors, but I am still not sure. We can reimplement it if this one is not working.

@pricop
Copy link
Contributor Author

pricop commented Jan 5, 2024

Sorry for the delay @ozdemirburak , I missed the notification.

The issue is almost resolved, it's just HEXA that's still not working as expected.

image

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

Successfully merging a pull request may close this issue.

3 participants