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

Figma connect files #4681

Merged
merged 10 commits into from
Jul 3, 2024
Merged

Figma connect files #4681

merged 10 commits into from
Jul 3, 2024

Conversation

lukasoppermann
Copy link
Contributor

@lukasoppermann lukasoppermann commented Jun 18, 2024

This PR adds some more code connect files to connect react components to figma implementations.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copy link

changeset-bot bot commented Jun 18, 2024

⚠️ No Changeset found

Latest commit: 2bc6b9e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Jun 18, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 91.07 KB (-0.01% 🔽)
packages/react/dist/browser.umd.js 91.31 KB (-0.01% 🔽)

@github-actions github-actions bot temporarily deployed to storybook-preview-4681 June 18, 2024 13:32 Inactive
@lukasoppermann lukasoppermann self-assigned this Jun 20, 2024
@lukasoppermann lukasoppermann force-pushed the figmaConnect-files branch 2 times, most recently from b9e6024 to a570d30 Compare June 28, 2024 12:54
@lukasoppermann lukasoppermann marked this pull request as ready for review July 2, 2024 11:15
@lukasoppermann lukasoppermann requested a review from a team as a code owner July 2, 2024 11:15
@github-actions github-actions bot temporarily deployed to storybook-preview-4681 July 2, 2024 13:14 Inactive
@joshblack joshblack added the skip changeset This change does not need a changelog label Jul 2, 2024
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Looks great 👍 Just left a question about the ESLint errors popping up and also saw that some of the files have TS errors in CI. Let me know if I can help out at all with those!

@lukasoppermann
Copy link
Contributor Author

Hey @joshblack I think we need to disable the type check for those files, or do you know how we can fix it? We can't add an as TYPE statement, as this will me transferred to figma.

@joshblack
Copy link
Member

@lukasoppermann just pushed up a commit to get TypeScript ✅ (hope that's okay, let me know if not!)

Let me know if that matches what you were thinking for these components or not 👀

}),
counter: figma.boolean('counter?', {
false: undefined,
true: figma.children('CounterLabel'),
true: figma.string('CounterLabel'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has to be reverted, we need children here.

<UnderlineNav.Item
aria-current={current}
counter={counter}
icon={leadingVisual ? () => leadingVisual : undefined}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this does not work I think as the code is not executed. But I'll check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it does not work and moving it to the figma.boolean section also does not.
I reached out to Figma for this.

@joshblack
Copy link
Member

@lukasoppermann I'll revert the commit based on your comments 👍 I'll add more info here for where the changes came from.

This has to be reverted, we need children here.

The type error is because counter only accepts string | number in the component but we're giving it a JSX.Element from figma.children.

We need double quotes, otherwise we end up with no quotes in figma.

The type error here is that the value being passed on as current is not 'page' but instead is '"page"' with the nested double quotes.

this does not work I think as the code is not executed. But I'll check.

The type error here is because leadingVisual does not accept JSX.Element, it expects a React.FunctionComponent which is where () => leadingVisual comes from.


My assumption is that we're running into an issue between how we want to represent it in Figma and how it's structured over in TS world 👀 Let me know how you would like to resolve these. My fear for disabling TypeScript on these examples is that TS seems to be catching snippets that would be invalid if someone tried to copy and use them in code due to the type mismatches.

@lukasoppermann lukasoppermann added this pull request to the merge queue Jul 3, 2024
Merged via the queue into main with commit 89c3a08 Jul 3, 2024
30 checks passed
@lukasoppermann lukasoppermann deleted the figmaConnect-files branch July 3, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants