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

Partial PR of blendmode value #193

Merged
merged 2 commits into from
Jun 29, 2022
Merged

Conversation

mattpilott
Copy link
Contributor

I've made a start on a PR, added the key and types but unsure what else i need to do to see the key in the JSON. I have had a hunt around but currently stuck so thought i'd submit what i have so far

@lukasoppermann
Copy link
Owner

Hey, thank you.

Before going into details, are all the blend modes supported in css or is there need for conversion?

@coveralls
Copy link

coveralls commented Mar 15, 2022

Pull Request Test Coverage Report for Build 2583296171

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 81.471%

Totals Coverage Status
Change from base Build 2582915197: -0.02%
Covered Lines: 538
Relevant Lines: 654

💛 - Coveralls

@mattpilott
Copy link
Contributor Author

These are all the supported ones in web

mix-blend-mode: normal|multiply|screen|overlay|darken|lighten|color-dodge|color-burn|difference|exclusion|hue|saturation|color|luminosity;

@mattpilott
Copy link
Contributor Author

At this point im just trying to get it to appear in the output 😅

@mattpilott
Copy link
Contributor Author

@lukasoppermann spent some time getting this working, its now correctly outputting fill blend modes.

@lukasoppermann
Copy link
Owner

lukasoppermann commented Mar 25, 2022

Hey @mattpilott sorry for the long wait.
From your end, is this ready to be merged?

It would be great if you could add some tests, to cover this new addition.

@mattpilott
Copy link
Contributor Author

Yes, it correctly outputs the blending mode in the json

@lukasoppermann
Copy link
Owner

Awesome, can you please add some test? I will check it again and once this is all done merge it. 🙏

@@ -136,7 +136,8 @@ const typographyValueTransformer = ({ name, values }) => ({

const colorValueTransformer = ({ fill }): StandardTokenDataInterface => ({
type: 'color' as StandardTokenTypes,
value: rgbaObjectToHex8(fill.value)
value: rgbaObjectToHex8(fill.value),
blendMode: fill.blendMode.toLowerCase()
Copy link
Owner

Choose a reason for hiding this comment

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

Here we need to replace line 140 with this:

blendMode: fill.blendMode?.toLowerCase() || 'normal'

Otherwise we get errors when no blendmode is set which is especially happening in gradients I think.

@@ -59,7 +60,8 @@ const extractFills = (paint): fillValuesType | gradientValuesType => {
return {
fill: {
value: convertPaintToRgba(paint),
type: 'color' as PropertyType
type: 'color' as PropertyType,
blendMode: paint.blendMode.toLowerCase()
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I missed this. Same needs to be done here paint.blendMode?.toLowerCase() || 'normal'

@lukasoppermann
Copy link
Owner

Hey, I missed one, added a comment.
Can you please also adjust the color, multi-color, gradient, colorAndGradient and gradientAndColor portions in extractedFigmaTokens.data.ts. to include blendMode as well as the fitting parts in transformedStandardTokens.data.ts

You should be able to see the issues when you run npm test

Thank you.

@lukasoppermann
Copy link
Owner

Hey @mattpilott I hope it works out. I just noticed, please remove the updates to the dist folder from the PR as well. Thank you.

@lukasoppermann lukasoppermann merged commit 1c46e61 into lukasoppermann:main Jun 29, 2022
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.

3 participants