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

feat: implement ColorPicker component #1795

Merged

Conversation

yvmunayev
Copy link
Contributor

fix: #1777

Changes proposed in this PR:

@nexxtway/react-rainbow

@commit-lint
Copy link

commit-lint bot commented Aug 25, 2020

Features

  • implement ColorPicker component (fcfd4a6)
  • implement default color section (007d6c6)

Bug Fixes

  • change saturation component (0cb522f)
  • unresolve import (1cc5d1f)
  • fix isHexColor test (fc6252e)
  • change the api (d28e630)
  • rearrange the structure of internal components (dd4d7eb)
  • import error of commons components (958b6fd)
  • add min-height to saturation component (59e2757)
  • emaple (34ab944)

Tests

Contributors

@yvmunayev, @TahimiLeonBravo

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@yvmunayev yvmunayev added the WIP work in progress label Aug 25, 2020
src/components/ColorPicker/saturation/index.js Outdated Show resolved Hide resolved
src/styles/helpers/color/rgbToHsv.js Show resolved Hide resolved
src/styles/helpers/color/hsvToRgb.js Show resolved Hide resolved
src/components/ColorPicker/defaultColors/index.js Outdated Show resolved Hide resolved
src/components/ColorPicker/saturation/styled.js Outdated Show resolved Hide resolved
src/components/ColorPicker/index.js Outdated Show resolved Hide resolved
src/components/ColorPicker/saturation/index.js Outdated Show resolved Hide resolved
src/components/ColorPicker/saturation/index.js Outdated Show resolved Hide resolved
src/components/ColorPicker/saturation/index.js Outdated Show resolved Hide resolved
src/components/ColorPicker/styled.js Outdated Show resolved Hide resolved
src/components/ColorPicker/index.js Outdated Show resolved Hide resolved
src/components/ColorPicker/styled.js Outdated Show resolved Hide resolved
src/components/ColorPicker/saturation/index.js Outdated Show resolved Hide resolved
src/components/ColorPicker/index.js Outdated Show resolved Hide resolved
src/styles/helpers/color/rgbToHsv.js Show resolved Hide resolved
@LeandroTorresSicilia
Copy link
Member

LeandroTorresSicilia commented Sep 1, 2020

@TahimiLeonBravo when using the color picker you need to pass a height to its container in order to see it like
Screen Shot 2020-09-01 at 11 50 59 AM

Instead if you don't pass a height then it looks like:
Screen Shot 2020-09-01 at 11 51 59 AM
where the gradient is hidden
this is the expected behavior?

const handleMouseDown = event => {
handleChange(event);
window.addEventListener('mousemove', handleChange);
window.addEventListener('mouseup', unbindEventListeners);

Choose a reason for hiding this comment

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

can we avoid setting listener in window and instead attach it to specific element to reduce its scope?

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 need to keep listening for the move event when it is outside the component so that the behavior is like the native component

@TahimiLeonBravo
Copy link
Collaborator

@TahimiLeonBravo when using the color picker you need to pass a height to its container in order to see it like
Screen Shot 2020-09-01 at 11 50 59 AM

Instead if you don't pass a height then it looks like:
Screen Shot 2020-09-01 at 11 51 59 AM
where the gradient is hidden
this is the expected behavior?

@LeandroTorresSicilia
this is not expected, I think it should have a min-height

@@ -0,0 +1,17 @@
import calculateBright from '../calculateBright';

describe('calculateBright', () => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

const wrapper = mount(<ColorPicker variant="colors-fixed" />);
const componets = ['Saturation', 'Hue', 'Alpha', 'Hex', 'Rgba'];

componets.forEach(component => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

const wrapper = mount(<ColorPicker defaultColors={[]} />);
const componets = ['Saturation', 'Hue', 'Alpha', 'Hex', 'Rgba'];

componets.forEach(component => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

const wrapper = mount(<ColorPicker variant="foo" />);
const componets = ['Saturation', 'Hue', 'Alpha', 'Hex', 'Rgba', 'DefaultColors'];

componets.forEach(component => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

decomposeColor,
} from '../../../../styles/helpers/color';

const Rgba = React.forwardRef((_props, ref) => {
Copy link

Choose a reason for hiding this comment

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

Function Rgba has 74 lines of code (exceeds 25 allowed). Consider refactoring.

@@ -0,0 +1,17 @@
import calculateSaturation from '../calculateSaturation';

describe('calculateSaturation', () => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

const wrapper = mount(<ColorPicker variant="colors-fixed" />);
const componets = ['Saturation', 'Hue', 'Alpha', 'Hex', 'Rgba'];

componets.forEach(component => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

const wrapper = mount(<ColorPicker defaultColors={[]} />);
const componets = ['Saturation', 'Hue', 'Alpha', 'Hex', 'Rgba'];

componets.forEach(component => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

const wrapper = mount(<ColorPicker variant="foo" />);
const componets = ['Saturation', 'Hue', 'Alpha', 'Hex', 'Rgba', 'DefaultColors'];

componets.forEach(component => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

decomposeColor,
} from '../../../../styles/helpers/color';

const Rgba = React.forwardRef((_props, ref) => {
Copy link

Choose a reason for hiding this comment

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

Function Rgba has 74 lines of code (exceeds 25 allowed). Consider refactoring.

@@ -0,0 +1,17 @@
import calculateBright from '../calculateBright';

describe('calculateBright', () => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

const wrapper = mount(<ColorPicker variant="colors-fixed" />);
const componets = ['Saturation', 'Hue', 'Alpha', 'Hex', 'Rgba'];

componets.forEach(component => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

const wrapper = mount(<ColorPicker defaultColors={[]} />);
const componets = ['Saturation', 'Hue', 'Alpha', 'Hex', 'Rgba'];

componets.forEach(component => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

const wrapper = mount(<ColorPicker variant="foo" />);
const componets = ['Saturation', 'Hue', 'Alpha', 'Hex', 'Rgba', 'DefaultColors'];

componets.forEach(component => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 4 locations. Consider refactoring.

decomposeColor,
} from '../../../../styles/helpers/color';

const Rgba = React.forwardRef((_props, ref) => {
Copy link

Choose a reason for hiding this comment

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

Function Rgba has 74 lines of code (exceeds 25 allowed). Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Sep 4, 2020

Code Climate has analyzed commit 224bb19 and detected 35 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 10
Duplication 25

View more on Code Climate.

@TahimiLeonBravo TahimiLeonBravo merged commit 45db3ed into nexxtway:master Sep 4, 2020
@yvmunayev yvmunayev deleted the implement-color-picker-component branch September 29, 2020 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: implement ColorPicker component
3 participants