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

1166 introduce color modifiers #1498

Merged
merged 46 commits into from Jan 11, 2023
Merged

1166 introduce color modifiers #1498

merged 46 commits into from Jan 11, 2023

Conversation

swordEdge
Copy link
Contributor

No description provided.

@swordEdge swordEdge requested review from six7 and LiamMartens and removed request for six7 December 29, 2022 07:24
@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2022

Commit SHA:6efbd1bd9bedb3600341f659afc40c08ca2a5459

Test coverage results 🧪

Code coverage diff between base branch:next and head branch: 1166-introduce-color-modifiers 
Status File % Stmts % Branch % Funcs % Lines
🔴 total 71.58 (-0.57) 63.71 (-0.6) 69.09 (-0.67) 72.01 (-0.59)
✨ 🆕 src/app/components/ColorPickerTrigger.tsx 66.67 100 0 66.67
✨ 🆕 src/app/components/ColorTokenForm.tsx 1.37 0 0 1.39
✨ 🆕 src/app/components/DropdownMenuRadioElement.tsx 25 100 0 33.33
✨ 🆕 src/app/components/TokenTooltip/SingleColorValueDisplay.tsx 100 25 100 100
🟢 src/app/store/models/tokenState.tsx 97.28 (0.73) 84.93 (-0.14) 100 (1.49) 97.16 (0.76)
✨ 🆕 src/constants/ColorModifierTypes.ts 100 100 100 100
✨ 🆕 src/constants/ColorSpaceTypes.ts 100 100 100 100
✨ 🆕 src/types/Modifier.ts 100 100 100 100
✨ 🆕 src/utils/convertModifiedColorToHex.ts 80 80 100 80
✨ 🆕 src/utils/modifyColor.ts 80 80 100 80
🟢 src/utils/alias/checkIfAlias.tsx 94.74 (5.27) 88.89 (11.11) 100 (0) 94.44 (5.55)
🟢 src/utils/alias/getAliasValue.ts 89.23 (0.9) 82.14 (1.58) 100 (0) 88.52 (1.02)
✨ 🆕 src/utils/is/isColorToken.ts 66.67 50 100 100
src/app/components/PropertyDropdownMenuRadioElement.tsx 25 100 0 33.33

@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2022

Commit SHA:0919394a797a67aa3fcd9ac29347fb2b41745cb9
Current PR reduces the test coverage percentage by 1 for some tests

@swordEdge swordEdge requested a review from six7 January 6, 2023 01:20
Copy link
Collaborator

@six7 six7 left a comment

Choose a reason for hiding this comment

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

some more things i encountered:

  • we're currently saving the value field as a string in JSON. can we save it as a number instead?
  • can you add something to the tooltip of color tokens that use a modifier? just beneath the value: lighten(0.5) / LCH so {operation}({value}) / {space}) or for mix: mix(#ffffff, 0.5) / LCH.

src/app/components/ColorTokenForm.tsx Outdated Show resolved Hide resolved
src/app/components/ColorTokenForm.tsx Outdated Show resolved Hide resolved
src/app/components/ColorTokenForm.tsx Outdated Show resolved Hide resolved
@swordEdge swordEdge requested a review from six7 January 7, 2023 17:36
import { TokenTypes } from '@/constants/TokenTypes';
import { SingleColorToken, SingleToken } from '@/types/tokens';

export function isSingleColorToken(token: SingleToken | any): token is SingleColorToken {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's call this isColorToken instead. Color tokens can only be single.

@swordEdge swordEdge requested a review from six7 January 9, 2023 09:29
@six7
Copy link
Collaborator

six7 commented Jan 9, 2023

@swordEdge One last thing: Can you make sure users can enter both 0.5 as well as 0,5 for the value? For me right now I have to enter 0,05 - I believe this depends on localization? Can we parse the input on change and change that to 0.5 always (the decimal point, not the decimal comma). Figma does the same if you type 0,5 in a width in Figma it converts to 0.5.

@six7
Copy link
Collaborator

six7 commented Jan 10, 2023

I found one more edge case:

If a user already had something else in their $extensions property (which will be likely given this is going to be how in the spec users are able to add extra metadata) we currently override the whole object

To reproduce:

  • add a $extensions field with some other identifier to a token
  • then add a color modifier to that token via the UI and save
  • notice how we removed the other $extensions value.

I'd expect that same $extensions property to still be there. We do not want to delete that for the user, we just want to append.

CleanShot.2023-01-10.at.23.42.46.mp4

Copy link
Collaborator

@six7 six7 left a comment

Choose a reason for hiding this comment

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

🎉

@six7 six7 merged commit 08ff1a6 into next Jan 11, 2023
six7 added a commit that referenced this pull request Jan 13, 2023
Co-authored-by: Jan Six <six7@github.com>
Co-authored-by: Jan Six <six.jan@gmail.com>
Co-authored-by: SorsOps <80043879+SorsOps@users.noreply.github.com>
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.

None yet

3 participants