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

Issue #37 : On editing a post, scroll partial to top of preview. #220

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

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