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.

Sign up for GitHub

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

Jump to bottom

fix(toggling renderInPlace) #58

Merged
merged 1 commit into from
Nov 3, 2017
Merged

fix(toggling renderInPlace) #58

merged 1 commit into from
Nov 3, 2017

Conversation

kybishop
Copy link
Owner

@kybishop kybishop commented Nov 2, 2017

Resolves #57

I'd like to add a test for this as I have vague memories of us being bitten by renderInPlace in the past.

@kybishop kybishop requested a review from pzuraq November 2, 2017 22:22
@pzuraq
Copy link
Collaborator

pzuraq commented Nov 2, 2017

I'm actually wondering now if we can replace both didUpdateAttrs and didInsertElement with a single call to didRender, which should always be where we want. Of course, it would have to be different for legacy (1.11), but I don't think it would be too bad

@kybishop
Copy link
Owner Author

kybishop commented Nov 2, 2017

As discussed in Slack, going to experiment a bit with didRender to see if it can suit our needs. If not, we'll move forward with this approach.

@kybishop kybishop force-pushed the allow-render-in-place-to-be-toggled branch from 80b799c to 6b15a9f Compare November 2, 2017 22:45
@kybishop
Copy link
Owner Author

kybishop commented Nov 2, 2017 •
edited
Loading

didRender indeed triggers on arg changes, even if those arg changes aren't directly represented in the template itself. PR updated to match.

@pzuraq should you want to support it, going to leave legacy to you.

EDIT: Didn't realize this would directly break legacy, thought just the bug itself would still be a problem (it might be, I'm not sure). Going to make sure the tests for legacy still pass before merging this.

@kybishop kybishop mentioned this pull request Nov 2, 2017
Merged
7 tasks
@kybishop kybishop force-pushed the allow-render-in-place-to-be-toggled branch 4 times, most recently from 5ced0e7 to b44e190 Compare November 2, 2017 23:14
@kybishop kybishop force-pushed the allow-render-in-place-to-be-toggled branch from b44e190 to 51fffa0 Compare November 3, 2017 00:34
@kybishop kybishop merged commit e777f3e into master Nov 3, 2017
@kybishop kybishop deleted the allow-render-in-place-to-be-toggled branch November 3, 2017 00:55
joeyrogues pushed a commit to joeyrogues/ember-popper that referenced this pull request Jun 13, 2018
@kybishop
fix(toggling renderInPlace) (kybishop#58)
5c7ec61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pzuraq pzuraq Awaiting requested review from pzuraq

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kybishop @pzuraq