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 "documentAccess": "dynamic-page" to manifest #323

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

0m4r
Copy link
Collaborator

@0m4r 0m4r commented Jan 3, 2025

Fixes #317

Two separate commits, one to address some leftover formatting, and one to address the improvement mentioned in #317.
Unit test have been add to to cover more code (github copilot to the rescue!)

@0m4r 0m4r self-assigned this Jan 3, 2025
@0m4r 0m4r requested a review from lukasoppermann January 3, 2025 15:30
@coveralls
Copy link

coveralls commented Jan 3, 2025

Pull Request Test Coverage Report for Build 12610310411

Details

  • 59 of 97 (60.82%) changed or added relevant lines in 11 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+7.4%) to 78.026%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ui/modules/urlExport.ts 1 4 25.0%
src/index.ts 0 5 0.0%
src/ui/modules/downloadJson.ts 0 5 0.0%
src/ui/modules/gitlabRepository.ts 1 26 3.85%
Files with Coverage Reduction New Missed Lines %
src/ui/modules/urlExport.ts 1 57.14%
Totals Coverage Status
Change from base Build 12580276052: 7.4%
Covered Lines: 654
Relevant Lines: 820

💛 - Coveralls

@lukasoppermann
Copy link
Owner

This looks very good. Thanks. 👍 Did you test it locally by building the app and running it in Figma?

I am on vacation so I can't test it for another week. But if you can confirm that it works, we can merge it. You just have to be 💯 sure as we can't so easily revert it I think.

I have no computer with me.

@0m4r
Copy link
Collaborator Author

0m4r commented Jan 4, 2025

This looks very good. Thanks. 👍 Did you test it locally by building the app and running it in Figma?

I am on vacation so I can't test it for another week. But if you can confirm that it works, we can merge it. You just have to be 💯 sure as we can't so easily revert it I think.

I have no computer with me.

hold on. I am setting this to draft.
I have run a test with a different Figma file again, and I have noticed I may have introduced an issue with the "type": "custom-fontStyle",.

I did have an issue with resolving the variable names with modes (or in general set with aliases) due to the new *Async functions. It should be resolved now.

I do not mind waiting until you run some tests too, just to maybe test with some more different figma files.

Enjoy your vacation!

@0m4r 0m4r marked this pull request as draft January 4, 2025 10:41
// get collection name and modes
const { variableCollectionId } = variable
const { name: collection, modes } = collections[variableCollectionId]
const variables = await Promise.all(localVariables?.filter((variable) =>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lukasoppermann I am not a big fan of all those Promise.all but I could not find a better solution.

What do you think?

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.

Add "documentAccess": "dynamic-page" to manifest.json
3 participants