fix createColorPicker ignores initial colour arg on chrome#8363
Conversation
Piyushrathoree
force-pushed
the
fix-createColorPicker-ignore-initial-color
branch
from
68c16e9 to
2271a30
Compare
|
@davepagurek could you please take a look |
| format: outputFormat | ||
| }); | ||
|
|
||
| if (format === '#rrggbb') { |
There was a problem hiding this comment.
Is this part of the code necessary? What does colorString look like after we pass hex into serialize?
There was a problem hiding this comment.
just checked it, seems like it is not necessary removing it right now.
There was a problem hiding this comment.
just checked it, seems like it is not necessary removing it right now.
when I try to remove it multiple tests are breaking , I'm not sure but it is converting even colors like 'rgb(100% 0% 40% / 0.8)' to string which is causing the problem and that's why this check it important.
There was a problem hiding this comment.
reading this more closely, is this just turning strings like #FFF into #FFFFFF? if so, could we do this without the regex and just use the indices of characters in the string since color serialization happens so frequently in p5?
There was a problem hiding this comment.
also I just checked this commit f465feb and it looks like that's applying the regex length thing to ALL color formats. I originally was asking if it was necessary to adjust the output of colorjs at all, e.g. removing the whole if statement including its contents.
There was a problem hiding this comment.
Hi @davepagurek ! I've updated the code to use string indices instead of regex. Now it directly accesses characters by their position (e.g., colorString[1], colorString[2]) and checks the length (=== 4 for short hex) rather than using regex pattern matching.
I 've tried and tested it working fine , please take look.
Piyushrathoree
force-pushed
the
fix-createColorPicker-ignore-initial-color
branch
from
5db49bd to
f465feb
Compare
This reverts commit f465feb.
Piyushrathoree
force-pushed
the
fix-createColorPicker-ignore-initial-color
branch
from
c6728c0 to
33b8ca3
Compare
|
@davepagurek please take a look . |
|
Hey @Piyushrathoree! Just checking in, are you still working on this one? I left some more comments on the thread above to try to clarify some things. |
|
Also there were some merge conflicts -- in |
got it will resolve soon |
I pulled the changes first and then pushed the new changes; I didn't encounter any issue. |
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Thanks for making those changes!
actually I was busy in my exams sorry for the delay |
|
3 participants
Resolves #8284
Changes:
I updated toString() in p5.Color.js to handle the #rrggbb format manually. It now calculates the values directly and ensures we always return a full 6-digit hex string. This fixes the issue for initial color breaking on createColorPicker.
Screenshots of the change:
PR Checklist
npm run lintpasses