Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Color select v2 #240

Merged
merged 18 commits into from
Mar 15, 2023
Merged

Color select v2 #240

merged 18 commits into from
Mar 15, 2023

Conversation

nathanbrud
Copy link
Collaborator

@nathanbrud nathanbrud commented Mar 10, 2023

What this PR does

Adds ColorSelect2 component which is a new version that eventually will replace ColorSelect.
The biggest change is in the spectrum and hue implementation to use react-colorful package that has better functionality and tests.

Currently is ColorSelect there's a critical bug where users cannot select colors from the edges, so black/white cannot be reached - ColorSelect2 is fixing it.

This PR merge is blocked by merging #235, after that merge the only changes between this PR and master will be under ColorSelect2 folder.

Important Changes

2 Tests are failing for ColorSelect2.
I've commented them out for now, I don't think it's a blocker because it's v2 and most teams can move the v2 without these props.

required was used to add an attribute to a div, to be honest I'm not sure why we would want to add required to a div.
Screen Shot 2023-03-10 at 8 35 16

name was also added to the same div, also not sure why we would do it.
Screen Shot 2023-03-10 at 8 35 24

Screenshots & Recordings

New.Recording.-.3_10_2023.8_47_21.AM.mp4

How it does that

  1. Remove all ...props usage so we actually know which props are used.
  2. Remove ColorSelectSpectrum and all not-needed state props.
  3. Add react-colorful and change a bit the css to fit the props ColorSelect is providing

Testing

Unit tests from original ColorSelect are used.
All stories have been copied to have both ColorSelect and ColorSelect2

@nathanbrud nathanbrud marked this pull request as ready for review March 10, 2023 07:11
@nathanbrud nathanbrud requested review from a team as code owners March 10, 2023 07:11
Copy link
Collaborator

@juliewongbandue juliewongbandue left a comment

Choose a reason for hiding this comment

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

Just some small nits, otherwise, lgtm.

@juliewongbandue
Copy link
Collaborator

Gonna tag @tylerthegrove for a second pair of eyes since this will be the first time we'll be including a third party dependency since they were first removed. 👀

Copy link
Contributor

@tylerthegrove tylerthegrove left a comment

Choose a reason for hiding this comment

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

Once we verify that the new dependency isn't included in rollup, I'm good with this.

package.json Show resolved Hide resolved
@juliewongbandue juliewongbandue requested review from tylerthegrove and removed request for tylerthegrove March 15, 2023 20:33
@juliewongbandue juliewongbandue merged commit b0a697a into vimeo:main Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants