Skip to content

Conversation

@gebilaoxiong
Copy link
Member

@gebilaoxiong gebilaoxiong commented May 31, 2017

issue: #5760

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@gebilaoxiong gebilaoxiong force-pushed the fix-empty-transition branch from 3f7dd9b to 2411c6f Compare May 31, 2017 18:23
@gebilaoxiong
Copy link
Member Author

gebilaoxiong commented May 31, 2017

It 's too late today and going to work tomorrow

I will fill the unit test in tomorrow ...

sorry

Copy link
Member

Choose a reason for hiding this comment

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

IMO 'div' is not a good idea

Copy link
Member Author

@gebilaoxiong gebilaoxiong Jun 1, 2017

Choose a reason for hiding this comment

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

But it is invisible, just a placeholder, used to replace comment

Copy link
Member

Choose a reason for hiding this comment

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

ping @yyx990803

@gebilaoxiong gebilaoxiong force-pushed the fix-empty-transition branch 2 times, most recently from 34f70dd to e0e5dad Compare June 4, 2017 13:59
@gebilaoxiong
Copy link
Member Author

gebilaoxiong commented Jun 4, 2017

@Kingwl

Now, I use a transition placeholder...

can you help me to review it, thx : )

@gebilaoxiong gebilaoxiong force-pushed the fix-empty-transition branch from e0e5dad to b2764bd Compare June 4, 2017 16:43
@gebilaoxiong gebilaoxiong force-pushed the fix-empty-transition branch from b2764bd to 072ac46 Compare June 4, 2017 16:45
Copy link
Member

@Kingwl Kingwl left a comment

Choose a reason for hiding this comment

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

LGTM

@yyx990803
Copy link
Member

Thanks @gebilaoxiong for the hard work. I found out a simpler fix in c3cdfcf but your PR helped a lot in identifying the problem.

@yyx990803 yyx990803 closed this Jun 5, 2017
@gebilaoxiong gebilaoxiong deleted the fix-empty-transition branch June 5, 2017 14:30
@gebilaoxiong
Copy link
Member Author

@yyx990803

Thanks, your code is pretty

@gebilaoxiong
Copy link
Member Author

gebilaoxiong commented Jun 5, 2017

@yyx990803 there is a small problem

c3cdfcf#diff-6964744ffd2d3536df0f526fb0e2c286R142

child.key = child.key == null
  ? id + child.tag
  : isPrimitive(child.key)
    ? (String(child.key).indexOf(id) === 0 ? child.key : id + child.key)
    : child.key

if child is async placeholder(comment) and it did not have the key

then the child key is __transition-1-undefined

84ffabb0-7036-4891-aec9-9034eb85580f

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