Skip to content

Conversation

@jhiswin
Copy link

@jhiswin jhiswin commented Mar 14, 2015

...ping into attribute map

issue #8

src/htmltojsx.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please put a single space after the // and capitalise the first word

@Daniel15
Copy link
Member

Sorry it took me so long to see this, I don't think I got an email about it :(

@jhiswin
Copy link
Author

jhiswin commented May 5, 2015

@Daniel15
Perhaps a patch based on importing Web IDL definitions like idl4js will work.
Also, I see some indication that React's attribute map may be deprecated at some point?
Do you happen to know what the plans are for that? It seems likely that they will be made distinct based on camel casing in the future in the JSX compiler?

@Daniel15
Copy link
Member

Daniel15 commented May 5, 2015

I think this change should be fine, but it looks like there's currently some merge conflicts. I'd be happy to merge it if you resolve the conflicts :)

@jhiswin
Copy link
Author

jhiswin commented May 7, 2015

@Daniel15 bump

src/htmltojsx.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Use triple equals for string equality, !==.

Can you add a test for the browser prefix case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be cleaner to use charAt since you know it's a String.

@jhiswin
Copy link
Author

jhiswin commented May 8, 2015

@ssorallen
Fixed it up; vendor prefixes should actually work correctly now.

src/htmltojsx.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

startsWith is not yet standard and not widely supported.

Copy link
Author

Choose a reason for hiding this comment

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

Replaced with regex test now

Copy link
Member

Choose a reason for hiding this comment

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

Fix the spacing here please - Looks like the it lines use tabs but everything else uses spaces

Copy link
Author

Choose a reason for hiding this comment

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

Alright, hopefully that fixes the last of it. Do you have some linting rules that could be put in here?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I'll have to see if I can figure out which linting tools to use, as I don't have experience with setting up linters.

Daniel15 added a commit that referenced this pull request May 10, 2015
issue #8 : case insensitive css attributes, import react's attribute map...
@Daniel15 Daniel15 merged commit ec7843d into reactjs:master May 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants