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(qwik): fix missing class attribute #1133

Merged
merged 5 commits into from
Apr 19, 2023
Merged

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Apr 18, 2023

Description

Add a short description of:

  • what changes you made,
  • why you made them, and
  • any other context that you think might be helpful for someone to better understand what is contained in this pull request.

This sort of information is useful for people reviewing the code, as well as anyone from the future trying to understand why changes were made or why a bug started happening.

@mhevery mhevery requested a review from samijaber as a code owner April 18, 2023 21:49
@vercel
Copy link

vercel bot commented Apr 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mitosis-fiddle ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 19, 2023 8:08pm

@@ -383,6 +383,8 @@ const componentMappers: {
}),
};
const properties = { ...block.properties };
if (block.id) properties['block-id'] = block.id;
if (block.class) properties['class'] = block.class;
Copy link
Contributor

Choose a reason for hiding this comment

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

worth pointing out that this 2nd line did not add anything to snapshots. Is it redundant? Or do snapshot tests not cover the usecase for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is the important bit. Will update the tests.

Comment on lines 229 to 250
expect(mitosis.trim()).toEqual(dedent`
import { For } from "@builder.io/mitosis";

export default function MyComponent(props) {
return (
<For each={state.submenusItem.menuItems}>
{(item, index) => (
<div
block-id="builder-ID"
class="class-id"
css={{
padding: "2px",
}}
>
text-content
</div>
)}
</For>
);
}
`);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use toMatchSnapshot instead?

@samijaber samijaber enabled auto-merge (squash) April 19, 2023 20:10
@samijaber samijaber merged commit d68f749 into main Apr 19, 2023
@samijaber samijaber deleted the pr-misko-missing-class branch April 19, 2023 20:12
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