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

Fix IllegalStateException on rgb percentage values #547

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonah1und1
Copy link
Contributor

Possible fix for #546.

@davewichers
Copy link
Collaborator

@spassarop - Can you look at this when you get a chance and let me know what you think? Is it a legit problem? Does the test case verify the program right? And is the fix right?

@spassarop
Copy link
Collaborator

@jonah1und1 I waited to be on my computer again and I can see the comments of a pending review I opened yesterday. I even tagged you in one. Do you see anything in this PR view? If you are logged in you should.

@jonah1und1
Copy link
Contributor Author

@spassarop Sadly, I am still unable to see your comments.

@kwwall
Copy link
Contributor

kwwall commented Jan 24, 2025 via email

* @return color value as int
*/
private int getColorIntValue(LexicalUnit param) {
if (param.getLexicalUnitType() == LexicalUnit.SAC_PERCENTAGE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jonah1und1 knowing we are dealing with building a string and it is possible to detect if we are handling a percentage, don't you think it would be most suitable and reliable in terms of matching the original input to reconstruct the percentage as a string? I know this affects your proposed method signature returning an int, but we always try to be as much equal as possible to the input if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I refactored the method.

@spassarop
Copy link
Collaborator

I think I had to submit the review. Can you see it now?

@jonah1und1
Copy link
Contributor Author

I think I had to submit the review. Can you see it now?

Yes, I can. Thank you.

@jonah1und1
Copy link
Contributor Author

@spassarop Thank you for your review!
I attended to your comments. Please feel free to take another look.

@jonah1und1 jonah1und1 requested a review from spassarop January 24, 2025 15:49
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.

4 participants