Skip to content

Conversation

@alexanderGugel
Copy link
Member

Alternative to #185 using Context.

Change to the way we retrieve the Context Size: getSize method has been moved from DOMRenderer to Context.

@michaelobriena

@DnMllr
Copy link
Contributor

DnMllr commented Jun 3, 2015

👍

I still believe that we need to figure out how to do this using mount state, but in the meanwhile this is good.

Copy link
Member Author

Choose a reason for hiding this comment

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

@DnMllr Are you ok with this? I think it's more intuitive to fetch the size from the actual context then the DOMRenderer. Sorry, didn't mean to put this in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

�I think this is good.

@michaelobriena
Copy link
Member

Looks like I found a bug in this, trying to repro.

@alexanderGugel
Copy link
Member Author

@michaelobriena Rebased. Can you reproduce the bug?

@michaelobriena
Copy link
Member

@alexanderGugel @pilwon 's example in #160 shows that it this solution breaks with first frame renderSize

https://gist.github.com/pilwon/eea5126d50090eb48204

Edit: Talked with Alex offline and confirmed that this solution did indeed cause different issues with renderSize.

@alexanderGugel
Copy link
Member Author

@michaelobriena I'm confused. This PR wasn't meant to address #160 . This PR only fixes the initial glitch. @browles solution hasn't been merged in yet.

IMO having the two PRs together should fix both issues, but I need to verify that.

@alexanderGugel
Copy link
Member Author

@michaelobriena Switched to naive display: 'none' solution. Needs more testing before merge.

@michaelobriena
Copy link
Member

I want to test this more, moving to the next minor.

@FarhadG
Copy link
Contributor

FarhadG commented Jun 8, 2015

Sounds like this will be merged in very soon?

@alexanderGugel @michaelobriena

@michaelobriena
Copy link
Member

@alexanderGugel this does not fix the issue. Run this branch with the seed project.

@alexanderGugel
Copy link
Member Author

@michaelobriena Are you sure you tested this properly? There is no way to test this using the seed project..... and everything works fine there. Please double check or be more specific.

@michaelobriena
Copy link
Member

Yes I tested this. If you check out your branch and then run the seed code you will see the spinning logo in the top left for a frame.

@alexanderGugel
Copy link
Member Author

This conflicts with some other changes. Not sure why. I think I haven't rebased this properly.

@alexanderGugel
Copy link
Member Author

Closing. Will reopen tomorrow.

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