Skip to content

Conversation

vmcj
Copy link
Member

@vmcj vmcj commented Aug 6, 2025

This is as a basis to detect problematic colors coming from the primary.

I can't think of any place where any consumer would ever want to display the colors with some opacity. So I wonder if it's intentional that the spec links to a document which has this.

@vmcj vmcj force-pushed the hex_shorthand_alpha branch from d0b078e to 83e8167 Compare August 6, 2025 20:25
*/
public static function parseHexColor(string $hex): array
{
// Ignore alpha in hexstrings
Copy link
Member

Choose a reason for hiding this comment

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

Why silently ignore the alpha channel?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might indeed a bit harsh to do this silently. Do we ever think we need the alpha channel? Otherwise I think I'll add a optional boolean for this behaviour (optionally also returning rgba instead of rgb), alternative is to check all calling functions and handle this there.

Copy link
Member

Choose a reason for hiding this comment

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

It's unlikely that we need it. But I'm either for supporting it properly or not allowing it at all to avoid user confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

The spec allows for sending such a string "#ff00ffaa" so we need to store it. I think my fix already makes that we never pick it up but I'll think about this some more. Maybe just supporting it is easier in that case.

@vmcj vmcj force-pushed the hex_shorthand_alpha branch 8 times, most recently from 5369d0c to f4e085c Compare August 9, 2025 14:46
@vmcj vmcj requested a review from meisterT September 4, 2025 10:36
Copy link
Member

@meisterT meisterT left a comment

Choose a reason for hiding this comment

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

Next to my minor comment, there are some phpcs errors left to fix. Otherwise good to merge.

@vmcj vmcj force-pushed the hex_shorthand_alpha branch 2 times, most recently from d4e80ec to c5bc5f1 Compare September 21, 2025 20:11
vmcj and others added 4 commits September 25, 2025 21:31
No functional change intended.
And add some more testing for those to also check for DOMjudge#123 format.

The functions using the strings have moved from the TwigExtension to
utils as the they don't really depend on Twig itself. This makes testing
easier and can use this in the future for other elements.

Co-authored-by: Tobias Werth <[email protected]>
The contest validation breaks when the contestproblem has an invalid color
PC^2 doesn't check the colors as they don't need them so mistakes in the
problemset.yaml would also be fed to the feed.
@vmcj vmcj force-pushed the hex_shorthand_alpha branch from be42623 to 333dc41 Compare September 25, 2025 19:38
@vmcj vmcj enabled auto-merge September 25, 2025 19:46
@vmcj vmcj added this pull request to the merge queue Sep 25, 2025
Merged via the queue into DOMjudge:main with commit f668c07 Sep 25, 2025
36 checks passed
@vmcj vmcj deleted the hex_shorthand_alpha branch September 25, 2025 20:29
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.

2 participants