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

fixed getStyleObjectFromRawCSS to handle css values with a colon #2814

Merged
merged 4 commits into from
Aug 13, 2022

Conversation

dosatross
Copy link
Contributor

@dosatross dosatross commented Aug 11, 2022

Currently if a css property has a colon in its value the rest of the value following the colon gets cut off.
Ran into this when using a font family with a prefix e.g. font-family: prefix:Arial

@vercel
Copy link

vercel bot commented Aug 11, 2022

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Aug 11, 2022 at 7:27AM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Aug 11, 2022 at 7:27AM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 11, 2022
@acywatson
Copy link
Contributor

Nice, thanks for writing a test!

const patch = style.split(':');
styleObject[patch[0].trim()] = patch[1].trim();
const [key, ...value] = style.split(':');
styleObject[key.trim()] = value.join(':').trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess another way to go at this would be Regex capture groups, which might be more performant than all this string manipulation. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im not great with regex but gave it a go - lmk what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch, the regex version looks much cleaner!

Copy link
Contributor

@acywatson acywatson left a comment

Choose a reason for hiding this comment

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

Looks right to me, pending CI pass

@@ -310,8 +310,8 @@ function getStyleObjectFromRawCSS(css: string): Record<string, string> {

for (const style of styles) {
if (style !== '') {
const patch = style.split(':');
styleObject[patch[0].trim()] = patch[1].trim();
const [key, value] = style.split(/:([^]+)/); // split on first colon
Copy link
Contributor

Choose a reason for hiding this comment

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

I had actually forgotten you could use the "^" character this way in JS RegEx!

@dosatross dosatross closed this Aug 12, 2022
@dosatross dosatross reopened this Aug 12, 2022
@thegreatercurve thegreatercurve merged commit 7831da7 into facebook:main Aug 13, 2022
thegreatercurve pushed a commit that referenced this pull request Nov 25, 2022
* fixed getStyleObjectFromRawCSS to handle css values with a colon

* use regex instead of string manipulation

* add test cases

* prettier formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants