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

Initial implementation of issue #2112 #2509

Merged
merged 1 commit into from
Nov 18, 2014

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented Nov 12, 2014

This is a first pass at the implementation of issue #2112, posted primarily to get early feedback. The change is not ready to be merged, though it shouldn't break anything.

TODO:

  • Read through the code to sanity check everything in this commit.
  • Verify the rerender logic will force an update when the context information changes.
  • Add unit tests to test the new functionality.

@sebmarkbage
Copy link
Collaborator

This needs a rebase but let's get it in sooner rather than later to avoid merge conflicts.

@jimfb jimfb force-pushed the use-parent-context branch 5 times, most recently from 4b93993 to a6c175d Compare November 18, 2014 00:50
@jimfb jimfb force-pushed the use-parent-context branch from a6c175d to 081feeb Compare November 18, 2014 01:03
jimfb added a commit that referenced this pull request Nov 18, 2014
@jimfb jimfb merged commit 89aaf73 into facebook:master Nov 18, 2014
@jimfb jimfb deleted the use-parent-context branch November 18, 2014 01:08
* @return {string} the HTML markup
*/
function renderToString(element) {
function renderToString(element, context) {
if(context === undefined) context = emptyObject;
Copy link
Member

Choose a reason for hiding this comment

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

:( @sebmarkbage I'm going to work on the style automation & linting as much as possible, but let's try to catch things like this (space before open paren, newlines with braces for ifs). Especially important for new contributors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@zpao No time. We needed to avoid another merge-hell.

However, I missed that renderToString still accepts a context. It shouldn't.

function renderToString(element, context) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renderToString fixed in #2565

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.

3 participants