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(color-picker): add color-preview #1395

Merged
merged 21 commits into from
Oct 21, 2021

Conversation

nooooooom
Copy link
Contributor

@nooooooom nooooooom commented Oct 18, 2021

close #1281.

@vercel
Copy link

vercel bot commented Oct 18, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tusimple/naive-ui/J3FEaGSvhay5TcYxQf1xgR8hkGsX
✅ Preview: https://naive-ui-git-fork-nooooooom-feat-color-picker-preview-tusimple.vercel.app

@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #1395 (b63fcca) into main (a98528f) will decrease coverage by 0.06%.
The diff coverage is 20.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1395      +/-   ##
==========================================
- Coverage   52.96%   52.89%   -0.07%     
==========================================
  Files         533      534       +1     
  Lines       13336    13354      +18     
  Branches     3807     3815       +8     
==========================================
+ Hits         7063     7064       +1     
- Misses       5151     5164      +13     
- Partials     1122     1126       +4     
Impacted Files Coverage Δ
src/color-picker/src/ColorPicker.tsx 48.05% <0.00%> (-0.72%) ⬇️
src/color-picker/src/ColorPreview.tsx 0.00% <0.00%> (ø)
src/color-picker/src/utils.ts 31.91% <28.57%> (-4.93%) ⬇️
src/color-picker/src/ColorPickerSwatches.tsx 79.31% <33.33%> (+20.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28f8eb...b63fcca. Read the comment docs.

function handleChange (e: Event): void {
// hex
const value = (e.target as HTMLInputElement).value
props.onUpdateColor?.(value.toLocaleUpperCase())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think toUpperCase is enough, why you chose toLocaleUpperCase()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I did not fully consider their usage scenarios, and I will replace it immediately.

@@ -1,5 +1,9 @@
# CHANGELOG

### Feats

- Add `n-color-preview` component, closes [#1281](https://github.com/TuSimple/naive-ui/issues/1281).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a prop to controll whether to show the native color picker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And for preview, should we always show it, or is it also controlled by prop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's okay to add a show-preview prop.

Copy link
Contributor Author

@nooooooom nooooooom Oct 20, 2021

Choose a reason for hiding this comment

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

Sorry that there was no update yesterday due to work reasons. This feature is now complete, but I don’t know whether my description in the document is appropriate.

Comment on lines 92 to 95
showPreview: {
type: Boolean,
default: false
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
showPreview: {
type: Boolean,
default: false
},
showPreview: Boolean,

export function convertColor (
value: string,
mode: ColorPickerMode,
originalMode?: ColorPickerMode | null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
originalMode?: ColorPickerMode | null
originalMode?: null

outline: none;
`, [
cE('fill', `
box-shadow: rgba(0, 0, 0, .15) 0px 0px 0px 1px inset, 0 0 4px var(--swatch-valid-color);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
box-shadow: rgba(0, 0, 0, .15) 0px 0px 0px 1px inset, 0 0 4px var(--swatch-valid-color);
box-shadow: rgba(0, 0, 0, .15) 0px 0px 0px 1px inset, 0 0 4px var(--swatch-valid-color);

What is --swatch-valid-color? For unconfigurable mutable color using inline style is enough.

Copy link
Contributor Author

@nooooooom nooooooom Oct 20, 2021

Choose a reason for hiding this comment

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

This approach is to display a shadow with the same color as the background when the swatch is focused.

The combination of them has a good visual effect, but I didn’t think of a good way to keep their colors consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's okay, keep it.

Copy link
Contributor Author

@nooooooom nooooooom Oct 20, 2021

Choose a reason for hiding this comment

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

On the color block on the left, I used filter to achieve a similar effect, this is their contrast picture. It can achieve similar visual effects by inheriting background.
image

use filter.
image

use box-shadow.
image

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 have thought about using color and currentColor before, but I want to reduce the dependency between CSS.

@@ -188,10 +218,18 @@ export default c([
cE('fill', `
width: 100%;
height: 100%;
background: var(--swatch-valid-color);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is --swatch-valid-color?

@07akioni
Copy link
Collaborator

@nooooooom rebase on the latest commit

@nooooooom
Copy link
Contributor Author

nooooooom commented Oct 21, 2021

@nooooooom rebase on the latest commit

I will do it now.

@07akioni 07akioni merged commit 9978d92 into tusen-ai:main Oct 21, 2021
@nooooooom nooooooom deleted the feat-color-picker-preview branch October 22, 2021 12:45
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.

color-picker 组件增加自定义预设颜色 最好能增加颜色吸取功能
4 participants