Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Conversation

@kienstra
Copy link
Contributor

Request For Review

Hi @westonruter,
Could you please review this pull request for Issue #37?

Editing a post field already triggers a refresh of its partial.
So in the refresh() method, define and call scrollPartialToTopOfPage.
Some themes have a margin-top of the #page element, like Twenty Sixteen.
And scrolling would hide part of the partial (see screenshot below).
So calculate the offsetTop of #page, to prevent this.
If this isn't a concern, this commit could be less verbose.

Ryan Kienstra added 2 commits August 16, 2016 19:39
Editing a post field will trigger a refresh of its partial.
In the refresh() method, define and call scrollPartialToTopOfPage.
Some themes have a margin-top of the #page element, like 2016.
And this scrolling would hide part of the partial.
So also calculate the offsetTop of #page, to prevent this.
If this isn't a concern, this commit could be much simpler.
@westonruter
Copy link
Contributor

@kienstra Yeah, there is a much simpler approach you can take here. Namely element.scrollIntoView(). If available, you could actually use element.scrollIntoViewIfNeeded().

Something to bear in mind is whether or not these methods trigger scroll events. If they don't, you'll probably want to manually trigger it on the window so that the scroll message will be sent to the parent frame to remember the scroll position.

@westonruter
Copy link
Contributor

@kienstra also, instead of adding this to the refresh method, I suggest instead that you extend the preparePlacement method, like so:

preparePlacement: function( placement ) {
    var partial = this;
    // @todo placement.container[0].scrollIntoViewIfNeeded(); ...
    return api.selectiveRefresh.Partial.prototype.preparePlacement.call( partial, placement );
}

@kienstra
Copy link
Contributor Author

Thanks, @westonruter. You raise good points here.

@westonruter westonruter added this to the 0.8 milestone Aug 17, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants