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

fix: preserve bound media query styles when converting builder to mitosis #1644

Merged
merged 8 commits into from
Nov 22, 2024

Conversation

liamdebeasi
Copy link
Contributor

Description

https://builder-io.atlassian.net/browse/ENG-7491

Bound media query styles are not preserved when converting Builder to Mitosis. This PR addresses the problem by ensuring that bound media query styles are included in the Mitosis node when bound using responsiveStyles.[SIZE].[CSS PROP].

Make sure to follow the PR preparation steps in CONTRIBUTING.md before submitting your PR:

  • format the codebase: from the root, run yarn fmt:prettier.
  • update all snapshots (in core & CLI): from the root, run yarn test:update
  • add Changeset entry: from the root, run yarn g:changeset and follow the CLI instructions. Alternatively, use the Changeset Github Bot to create the file.

Copy link

changeset-bot bot commented Nov 18, 2024

🦋 Changeset detected

Latest commit: 1cb7328

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@builder.io/mitosis Patch
@builder.io/mitosis-cli Patch

Not sure what this means? Click here to learn what changesets are.

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

'responsiveStyles.small.left': 'state.left',
'responsiveStyles.small.top': 'state.top',
'responsiveStyles.large.color': 'state.color',
'style.fontSize': 'state.fontSize',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to show that the presence of responsiveStyles. doesn't interfere with other styles bound using style.

Copy link

nx-cloud bot commented Nov 18, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 1cb7328. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

@liamdebeasi liamdebeasi marked this pull request as ready for review November 18, 2024 22:12
const [_, size, prop] = key.split('.');
const mediaKey = `@media (max-width: ${sizes[size as Size].max}px)`;

const objKey = `'${mediaKey}'`;
Copy link

Choose a reason for hiding this comment

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

just out of curiosity more than anything, does wrapping this do anything as opposed to using responsiveStyles[mediaKey]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key needs to be surrounded in quotes because it has spaces/special characters. Here's what it looks like without the extra quotes:

- Expected
+ Received

  {
    "style": {
      "bindingType": "expression",
-     "code": "{ fontSize: state.fontSize, '@media (max-width: 640px)': {\"left\":\"state.left\",\"top\":\"state.top\"}, '@media (max-width: 1200px)': {\"color\":\"state.color\"}, }",
+     "code": "{ fontSize: state.fontSize, @media (max-width: 640px): {\"left\":\"state.left\",\"top\":\"state.top\"}, @media (max-width: 1200px): {\"color\":\"state.color\"}, }",
      "type": "single",
    },
  }

Though this is a good reminder that I should document why this is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

documented here: 8d7ae81

also switched to using double quotes for consistency with the rest of the object (shouldn't make any functional difference)

@liamdebeasi
Copy link
Contributor Author

Added the follow up ticket per our discussion earlier today: https://builder-io.atlassian.net/browse/ENG-7569

@liamdebeasi liamdebeasi enabled auto-merge (squash) November 22, 2024 17:19
@liamdebeasi liamdebeasi merged commit 10a168d into BuilderIO:main Nov 22, 2024
8 checks passed
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.

2 participants