Skip to content

Conversation

@ali-khalili
Copy link
Contributor

There is an issue when handling incorrect RGB colors which have lesser character numbers than 6. In this situation, we should probably either throw an exception OR define a fallback value for the color.
In this PR, I have updated the code to use RGB(0,0,0) as the fallback color when this situation happens.
Thank you

Handle incorrect inputs color which have less than 6 chars
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Hi @ali-khalili,

Thanks for this. Good catch.

I think when an invalid property value is encountered, an UnexpectedTokenException should be thrown. This will be caught upstream so that the invalid property is dropped (what most browsers do), rather than resulting in a property with a now-valid value (causing a change of behaviour if the rendered CSS is used in place of the input CSS).

Also, could you add an entry to CHANGELOG.md (newest first within the 'Fixed' section)?

Thanks <3

@oliverklee, @sabberworm, do you spot any other issue?

@JakeQZ JakeQZ added the bug label Feb 19, 2024
@oliverklee oliverklee changed the title handle exception of parsing incorrect RGB colors [BUGFIX] Handle incorrect RGB colors better Feb 20, 2024
Copy link
Collaborator

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Yes, please let's discard invalid rules instead of falling back to some default.

Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Thanks for contributing 👍

Copy link
Collaborator

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Thanks! ❤️

@oliverklee oliverklee merged commit 6a5d91d into MyIntervals:main Feb 20, 2024
@ali-khalili ali-khalili deleted the feature_fix-incorrect-rgb branch February 20, 2024 22:13
@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 21, 2024

It didn't go unnoticed that you had to wait for the CI checks to be triggered by a maintainer. Thanks for persevering. I hope we can imporove the process for future contributions/contributors (#486).

@oliverklee
Copy link
Collaborator

@JakeQZ @sabberworm Is this something that would make sense to backport to 8.x?

@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 21, 2024

Is this something that would make sense to backport to 8.x?

I think so:

  • it's a bug fix;
  • it's quite a small change so should be fairly straightforward to merge.

oliverklee pushed a commit that referenced this pull request Feb 22, 2024
Handle incorrect inputs color which have less than 6 chars
@oliverklee
Copy link
Collaborator

Done: #490

JakeQZ

This comment was marked as duplicate.

@JakeQZ

This comment was marked as off-topic.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants