-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Polish Label Color Picker #12464
Polish Label Color Picker #12464
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good. I have one suggestion regarding the ColorPicker and making it more of a controlled component along with a few other things.
} | ||
|
||
export default class ColorPicker extends Component<Props, State> { | ||
public static defaultProps: DefaultProps = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noice!
const properties = {...this.state.label.properties, color} | ||
const label = {...this.state.label, properties} | ||
|
||
this.setState({label}) | ||
} | ||
console.log(hexcodeStatus) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
private handleSelectColor = (color: string): void => { | ||
this.setState({color}) | ||
} | ||
|
||
private handleChangeInput = (_: ChangeEvent<HTMLInputElement>): void => { | ||
// console.log('changing: ', e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this console.log commetn be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is actually in there on master
. It makes me wonder if the settings page is even doing anything
} | ||
} | ||
|
||
componentDidUpdate(prevProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should prevProps be typed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES
|
||
componentDidUpdate(prevProps) { | ||
if (prevProps.selectedHex !== this.props.selectedHex) { | ||
this.setState({inputValue: this.props.selectedHex}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it necessary to keep track of the hex value in state? it seems like this will be creating two sources of truth? could all the places that state instead call the onSelect() prop?
@ischolten I refactored this thing a bit so it's a controlled component. I only store hexcode validation in the color picker state so that doesn't need to be repeated everywhere this component is used |
Closes #10810
ColorPicker
componentColorPicker
componentChecklist