Migrate ColorInput component to typescript #3589
Conversation
jespino
added
2: Dev Review
ghost
requested review from
mickmister and
stylianosrigas
and removed request for
a team
| checkClick = (e) => { | ||
| const colorPickerDOMNode = ReactDom.findDOMNode(this.colorPicker); | ||
| if (!colorPickerDOMNode || !colorPickerDOMNode.contains(e.target)) { | ||
| private checkClick = (e: MouseEvent): void => { |
There was a problem hiding this comment.
React.MouseEvent<HTMLElement> is often used in favor of MouseEvent. In my experience, using React.MouseEvent<HTMLElement> usually results in less casting when accessing properties of the event.
Thoughts @jespino?
There was a problem hiding this comment.
In this case we are not talking about a React.MouseEvent, it is just a MouseEvent, because is connecting directly to the document using addListener, not through onClick. I think the proper types is MouseEvent and the casting is needed because the MouseEvent target field is not exactly a Element, is an EventTarget which is a subset of Element based on the event type.
Co-Authored-By: Michael Kochell <[email protected]>
|
👍 looks like it just needs needs QA review |
|
@mickmister yes, I didn't added anybody because there is no rush about it, I added the label and I'll wait until anybody is free to take a look :) |
|
@jespino Yes, we should disable the alpha channel for the colorpicker. |
|
@jespino Please see Asaad's comment -this is good to go from my perspective |
|
@asaadmahmood @wiersgallak Great, I going to create a ticket for that, and I add that change in another PR (This PR is already reviewed and actually I want to use this as an example for a campaign, so, the less changes, the better). |
|
Does this one have a milestone / fix version? And will / should there be any QA test steps? |
|
@lindalumitchell Not necessarily have a milestone/fix version, whenever it gets merged is going to determine the fix version. I added the QA steps to the ticket. |
lindalumitchell
added
the
Setup Cloud Test Server
|
Tested each color picker in Display > Theme, and all worked as expected. No issues found. Removing QA Review label. |
hanzei
added
4: Reviews Complete
amyblais
added
Changelog/Not Needed
lindy65
added
Tests/Not Needed
* Migrate ColorInput component to typescript * Being specific about the @types/react-color version * Changing ColorInput to PureComponent. Co-Authored-By: Michael Kochell <[email protected]>
* Migrate ColorInput component to typescript * Being specific about the @types/react-color version * Changing ColorInput to PureComponent. Co-Authored-By: Michael Kochell <[email protected]>
Summary
Migrate ColorInput component to typescript
Ticket Link
MM-18192
Notes for QA
This is used in the custom theme color selector.
Notes for PMs
My perception is we are not allowing (using) transparency but the widget is showing the alpha channel, there is a setting of the component that allow us to disable the alpha channel in the selector, maybe we should use that to not allow play with the alpha channel if this is not used at all.