Skip to content

fix object styles not escaped#4887

Merged
matthewp merged 6 commits intowithastro:mainfrom
Calvin-LL:patch-2
Oct 4, 2022
Merged

fix object styles not escaped#4887
matthewp merged 6 commits intowithastro:mainfrom
Calvin-LL:patch-2

Conversation

@Calvin-LL
Copy link
Copy Markdown
Contributor

@Calvin-LL Calvin-LL commented Sep 27, 2022 •
edited
Loading

Changes

Before this PR:

<div
  style={{
    backgroundImage: `url("test.png")`,
  }}
>
</div>

would be rendered as

<div
  style="background-image: url("test.png")"
>
</div>

which is invalid HTML, since the quotes in url are not escaped.

After the fix in this PR, the output is:

<div
  style="background-image: url(&#34;test.png&#34;)"
>
</div>

Also found a bug where shouldEscape isn't passed down to toAttributeString and I fixed it.

Testing

Added astro-object-style test

Docs

Minor fix

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Sep 27, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 882c4c8

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

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Sep 27, 2022
matthewp
matthewp requested changes Sep 27, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@matthewp matthewp 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, would you mind adding a test? See packages/astro/test/fixture for examples of what our tests look like.

Also a changeset is needed:

pnpm changeset

@Calvin-LL
Copy link
Copy Markdown
Contributor Author

Calvin-LL commented Sep 27, 2022 •
edited
Loading

Looks great, would you mind adding a test? See packages/astro/test/fixture for examples of what our tests look like.

Also a changeset is needed:

pnpm changeset

Thanks! I've added tests and ran changeset.

Update: just rebased on to the lastest HEAD on main, looks like the HEAD itself isn't passing checks either, will rebase again once fixed

@Calvin-LL Calvin-LL requested a review from matthewp September 27, 2022 22:36
@Calvin-LL Calvin-LL force-pushed the patch-2 branch 3 times, most recently from 3e48e42 to 882c4c8 Compare September 29, 2022 06:30
@matthewp
Copy link
Copy Markdown
Contributor

matthewp commented Sep 29, 2022

@Calvin-LL this is just a flakey test. Will rerun and it will likely pass. Thank you!

@matthewp matthewp merged commit 37cb2fc into withastro:main Oct 4, 2022
@astrobot-houston astrobot-houston mentioned this pull request Oct 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewp matthewp matthewp approved these changes

Assignees

No one assigned

Labels

pkg: astro Related to the core `astro` package (scope)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Calvin-LL @matthewp