Skip to content

Conversation

@waldreiter
Copy link
Contributor

Target is the last option missing in the readme.

@waldreiter
Copy link
Contributor Author

I found another missing option: --use-provides-module
Is this only used by facebook internally or should it be documented too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Markdown files don't usually have
elements, do they? Usually markdown is intended to be both human and machine readable, to the extent possible, right? cc @zpao who will know best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is allowed to have html elements. At least <iframe> is already used in the docs for inline videos. Here this is explained: Inline HTML.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but is it good style? Is it what we want to do here? My understanding (which is admittedly limited) is that we want to avoid HTML in the markdown files whenever possible. In the case of a linebreak, it seems avoidable, but maybe it's necessary/desirable here because we're in a table. Another option would be to use a coma or other form of punctuation. instead of a br. I don't have a strong preference here, mostly just flagging it for @zpao to take a look and make a call.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, that's allowed. Note, you can always view the rendered output on github, even in PRs.

@zpao zpao added this to the 0.13.2 milestone Apr 9, 2015
@zpao
Copy link
Member

zpao commented Apr 9, 2015

Thanks!

And don't worry about the --use-provides-module option. It really shouldn't be exposed but I don't really feel like making that happen with commoner, so meh.

zpao added a commit that referenced this pull request Apr 9, 2015
Add target option to npm readme
@zpao zpao merged commit 857736d into facebook:master Apr 9, 2015
zpao added a commit to zpao/react that referenced this pull request Apr 13, 2015
Add target option to npm readme
zpao added a commit to zpao/react that referenced this pull request Apr 17, 2015
Add target option to npm readme
zpao added a commit to zpao/react that referenced this pull request Apr 18, 2015
Add target option to npm readme
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.

4 participants