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: Add Color Picker #253

Merged
merged 30 commits into from
Sep 2, 2021
Merged

feat: Add Color Picker #253

merged 30 commits into from
Sep 2, 2021

Conversation

saaaaaally
Copy link
Contributor

@saaaaaally saaaaaally commented Aug 19, 2021

Added basic and advanced color picker.

image

@mayank99
Copy link
Contributor

3 major things from my perfunctory look before I do any "review":

  • use variables, not magic numbers
  • read about CSS/Sass specificity and nesting
  • read about focus vs focus-visible (browser support/fallback, respecting input modality, etc)

1 minor thing:

  • run prettier ("Format Document" Alt+Shift+F) before pushing

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

Expanding a bit on my points above.

src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
backstop/tests/color-picker.html Outdated Show resolved Hide resolved
backstop/tests/color-picker.html Outdated Show resolved Hide resolved
backstop/tests/color-picker.html Outdated Show resolved Hide resolved
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

I think the advanced part could use some restructuring/cleanup.

Maybe @FlyersPh9 could provide some guidance on the values/visuals

src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
backstop/tests/color-picker.html Show resolved Hide resolved
src/color-picker/color-picker.scss Show resolved Hide resolved
src/color-picker/color-picker.scss Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
backstop/scenarios/color-picker.js Outdated Show resolved Hide resolved
@FlyersPh9
Copy link
Collaborator

We discussed in a call to have the basic color picker's width be responsive. If there are only 2 color swatches, the width will shrink to fit the 2 swatches. If there are above 10 swatches, we will wrap the swatches to a new line so that only 10 swatches are on a row.

For the advanced color picker, the width will always be the same no matter how many saved colors are listed.

@FlyersPh9
Copy link
Collaborator

We also discussed in a call setting a max-height on the color swatches. If we have a scenario where we have dozens of rows that we will only display so many rows (undecided on how many) and will apply a vertical scrollbar.

@saaaaaally saaaaaally marked this pull request as ready for review August 26, 2021 16:39
@saaaaaally saaaaaally requested a review from a team as a code owner August 26, 2021 16:39
@saaaaaally saaaaaally requested review from a team and gretanausedaite and removed request for a team August 26, 2021 16:39
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
backstop/tests/color-picker.html Outdated Show resolved Hide resolved
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

This PR is looking good and pretty close to completion!

I would recommend getting started on the React implementation (by linking the repos) while you finalize this. You might actually find that some CSS changes need to be made as you're working on React.

src/style/variables.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
backstop/tests/color-picker.html Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
@saaaaaally
Copy link
Contributor Author

@mayank99 @veekeys @gretanausedaite I think all the comments should be resolved now, please take a look and let me know if there is still something that needs to be fixed

backstop/tests/color-picker.html Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
src/color-picker/color-picker.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

Looks good 👍 (unless we need some changes for React)

For some reason, the default color picker tests keep failing (despite no visual differences), so try re-running and/or updating images just in case.

@saaaaaally saaaaaally merged commit a6e2e45 into main Sep 2, 2021
@saaaaaally saaaaaally deleted the saally/color-picker branch September 2, 2021 17:51
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.

5 participants