Skip to content

Improve spec compliance for text-decoration #65

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

Merged
merged 2 commits into from
Feb 9, 2018

Conversation

jacobp100
Copy link
Contributor

No description provided.

) {
lines.push(tokenStream.lastValue)
if (lines[0] === lines[1]) {
throw new Error('Expected two different text decoration line styles')
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding a test for this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh. Not even sure why I added that. I should probably just let RN handle the invalid values

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, to me it seems a bit weird that some CSS parsing errors are thrown by css-to-react-native and others go all the way to RN. Of course in both cases the user will see the error.

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 think maybe the cut-off should be is if the parsing is ambiguous or incomplete we throw an error, otherwise let RN do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that make sense

Copy link
Contributor

@kristerkari kristerkari left a comment

Choose a reason for hiding this comment

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

Looks good

@kristerkari
Copy link
Contributor

Oh yeah there is still the ongoing discussion about reordering values in the other PR, so if something is changed there it needs to be changed here too.

#66 (comment)

@jacobp100 jacobp100 merged commit 1c0abd2 into master Feb 9, 2018
@jacobp100 jacobp100 deleted the text-decoration-fixes branch February 9, 2018 21:39
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