Closed kubahasek closed 2 years ago
Name | Link |
---|---|
Latest commit | af987056443c137a39438cd188007a1a5e9974f9 |
Latest deploy log | https://app.netlify.com/sites/developertools-tech/deploys/634dfa7c945fe600082a70f3 |
Deploy Preview | https://deploy-preview-63--developertools-tech.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
@kubahasek I think there is some complexity here around having duplicate functionality in the color picker package and the work you have done (rbg and hex inputs). Would it be too difficult to switch to something a bit simpler like react-colorful?
@dlford I can try that out, see if it will help 👍
@dlford
It has made the code a bit simpler as I don't need to states but only one to maintain the state, but the issue with changing the value in the browser sadly still persists.
@kubahasek No worries, it looks to be mostly working, I'll take a look at it and see what we can do.
@kubahasek I believe this is working as expected now, I had to make the color value states strings and conditionally cast them to the appropriate types if they were complete. I also made a few other tweaks and merged with the dev
branch to resolve the conflicts, you'll need to npm install
again as some dependencies have changed.
I think all we need now are some tests, let me know if you need anything else! 😁
One more thing I just noticed, we need to stack the layout on mobile so it's still usable.
@dlford That's awesome, thank you! Sorry I couldn't get it all done myself.
I'll try and get the tests done tomorrow 😄
@dlford
I've done both things, but the tests are failing at the moment. I can't see an apparent problem right now, so unsure why it wouldn't work, I used basically the same approach as all the others
Will try and get fixed tomorrow.
@kubahasek No worries! Let me know if you want me to take a look.
@dlford Really hate to ask, but if you could take a look, I'd appreciate it a lot.
@kubahasek no problem! I'll take a look when I get some time
@kubahasek I fixed the tests, instead of expect(hex.innerHTML).toBe('#000000');
, you want expect(hex).toHaveValue('#000000');
Also made a few UI tweaks, I think maybe a nice solid block of the chosen color above the picker would be nice if you'd like to add that? I don't mind adding it if you don't want to, you've already spent a bunch of time on this.
Thanks!
Also, an eye dropper! - https://developer.mozilla.org/en-US/docs/Web/API/EyeDropper (this would need to check for support before showing the button, like the paste buttons do).
Same thing applies, you've spent a bunch of time on this already so I don't mind doing that work myself, if you'd like to get your work merged just make this PR "ready for review" and I'll merge it in 😁
@dlford
I would love to help out more, I just need one more PR after this one to make my goal in Hacktoberfest. Would it be okay with you to separate the other two things as "Improvements to the color tool" or similar? I would love to work on it as it seems very interesting (Eyedropper specifically)
If not I understand! 😄
@kubahasek Cool! Please claim #66 if you'd like to!
Type of Pull Request
Related Issue #s or links (if any): #12
Description of Changes
Adding a color conversion tool