Skip to content

[BUGFIX] Handle incorrect RGB colors better #485

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

Merged
merged 6 commits into from
Feb 20, 2024

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