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

Polish Label Color Picker #12464

Merged
merged 12 commits into from
Mar 11, 2019
Merged

Polish Label Color Picker #12464

merged 12 commits into from
Mar 11, 2019

Conversation

alexpaxton
Copy link
Contributor

@alexpaxton alexpaxton commented Mar 8, 2019

Closes #10810

  • Introduce Clockface ColorPicker component
    • Can be used anywhere single color selection needs to happen
    • Has hexcode validation baked in
    • Can optionally report the validation status of itself
  • Replace color selection dropdown in label create/update overlay with new ColorPicker component

label-color-picker

Checklist

  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Sign CLA (if not already signed)

Copy link
Contributor

@ischolten ischolten left a 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 = {
Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be removed?

Copy link
Contributor Author

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should prevProps be typed?

Copy link
Contributor Author

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})
Copy link
Contributor

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?

@alexpaxton
Copy link
Contributor Author

@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

@alexpaxton alexpaxton merged commit 1c72fb3 into master Mar 11, 2019
@alexpaxton alexpaxton deleted the clockface/color-picker branch March 11, 2019 17:27
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.

2 participants