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

Support high contrast colors #157

Merged
merged 25 commits into from
Apr 2, 2022

Conversation

alexey1312
Copy link
Contributor

Solution for this issue

add lightHighContrastFileId and darkHighContrastFileId
add lightHCModeSuffix and darkHCModeSuffix
and the AssetPair
Copy link
Collaborator

@subdan subdan left a comment

Choose a reason for hiding this comment

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

Sources/FigmaExportCore/Processor/AssetsProcessor.swift Outdated Show resolved Hide resolved
Sources/FigmaExport/Loaders/ColorsLoader.swift Outdated Show resolved Hide resolved
Sources/FigmaExport/Loaders/ColorsLoader.swift Outdated Show resolved Hide resolved
Sources/FigmaExportCore/Processor/AssetsProcessor.swift Outdated Show resolved Hide resolved
Sources/FigmaExportCore/Processor/AssetsProcessor.swift Outdated Show resolved Hide resolved
Sources/FigmaExportCore/Processor/AssetsProcessor.swift Outdated Show resolved Hide resolved
Sources/FigmaExportCore/Processor/AssetsProcessor.swift Outdated Show resolved Hide resolved
@alexey1312 alexey1312 requested a review from subdan March 24, 2022 18:39
Copy link
Collaborator

@subdan subdan left a comment

Choose a reason for hiding this comment

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

  1. Fix typo highthigh
  2. Fix issue I point in the comment about dark appearance.
  3. Add unit tests for XcodeColorExporter which checks exporting high contrast colors for useSingleFile is true and false.
  4. I tried to run FigmaExport for Example project and it fails to export with the following error: Bad asset name.... You must fix this issue.

Sources/XcodeExport/XcodeColorExporter.swift Outdated Show resolved Hide resolved
@alexey1312
Copy link
Contributor Author

1 - Fix typo hight → high ✅
2 - Fix issue I point in the comment about dark appearance. ✅
3 - Add unit tests for XcodeColorExporter which checks exporting high contrast colors for useSingleFile is true and false.
It seems that testing useSingleFile in XcodeColorExporter is not very correct, since different areas of responsibility.
I think we need to write tests on the ColorLoader, but for this it needs to be refactored.
I checked in Playgrounds, the filter for useSingleFile works correctly. ✅
4 - I tried to run FigmaExport for Example project and it fails to export with the following error: Bad asset name.... You must fix this issue. ✅

@alexey1312 alexey1312 requested a review from subdan March 30, 2022 10:09
Sources/XcodeExport/Model/XcodeExportExtensions.swift Outdated Show resolved Hide resolved
let darkHCSet: Set<AssetType> = foundDuplicate(assets: darkHC, errors: &errors)
// AssetNotFoundInLightPalette
checkSubtracting(firstAsset: lightSet, secondAsset: darkSet, errors: &errors)
checkSubtracting(firstAsset: lightSet, secondAsset: lightHCSet, errors: &errors)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Method checkSubtracting can generate darkAssetsNotFoundInLightPalette error only but you specified light and light high contrast colors.

@alexey1312 alexey1312 requested a review from subdan March 31, 2022 01:13
@subdan subdan merged commit 1438b90 into RedMadRobot:master Apr 2, 2022
@subdan subdan mentioned this pull request Apr 2, 2022
@subdan
Copy link
Collaborator

subdan commented Apr 2, 2022

Thanks for the PR, @alexey1312! I've published a new version of FigmaExport with this feature.

@alexey1312
Copy link
Contributor Author

alexey1312 commented Apr 2, 2022

Thanks for the code review, @subdan ! 🙏

@alexey1312 alexey1312 deleted the feature/highContrast branch April 2, 2024 15:40
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

2 participants