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: Removed extra double quotes from computed style in shiki code component #8035

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

fyndor
Copy link
Contributor

@fyndor fyndor commented Aug 11, 2023

Changes

Removed an invalid extra double quote that is added to the shiki wrapper component called Code.

Testing

Sorry, I don't have the time to learn how to properly create unit tests for this within your ecosystem. Sorry if this is unacceptable. The test I would add I guess would be whether the style attribute is properly formed without the extra double quote. I am not sure how to do that.

Docs

I would assume no document changes should be needed for this. It currently creates malformed html, but at least Chrome still renders properly even with the malformed HTML. This should not affect the functionality other than possibly make stricter browsers (if they exist) work properly.

@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2023

🦋 Changeset detected

Latest commit: d4e9f16

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 Aug 11, 2023
@fyndor fyndor changed the title Removed extra double quotes from computed style in shiki code component fix: Removed extra double quotes from computed style in shiki code component Aug 11, 2023
@fyndor
Copy link
Contributor Author

fyndor commented Aug 11, 2023

Not sure what that CI error means

@matthewp
Copy link
Contributor

In what case does this create malformed HTML? Can you post an example?

@fyndor
Copy link
Contributor Author

fyndor commented Aug 11, 2023

I would say in all cases, but I guess technically if you pass null to the wrap prop then it wouldn't cause malformed html. Not sure the purpose of wrap == null.

<Code code="some code" />
<Code code="some code" wrap={false} />
<Code code="some code" wrap={true} />

These should all add an extra double quote in html. But I guess the following would lead to valid html:

<Code code="some code" wrap={null} />

The code surrounding adding extra styling depending on the wrap prop value adds a double quote at the end of the appended style string. Then in the return statement, the style string variable is inserted between two double quotes. So if wrap != null, then there is always an extra double quote at the end of the style attribute. If you check out the commit, the It's pretty obvious there is an erroneous double quote at the end of those two lines. I was looking at the outputted html when I discovered the issue then backtracked it to the code fixed by this commit.

.changeset/tall-owls-act.md Outdated Show resolved Hide resolved
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Looks like a simple fix to me. Thanks for catching this!

@bluwy bluwy merged commit a12027b into withastro:main Aug 15, 2023
@astrobot-houston astrobot-houston mentioned this pull request Aug 15, 2023
@fyndor
Copy link
Contributor Author

fyndor commented Aug 15, 2023

No problem. This might be my first non-documentation open source contribution :D Glad to help improve Astro in my small way. It's a great project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants