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.

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

pat-scroll: Implement selector:top #722

Merged
merged 2 commits into from
May 21, 2020
Merged

pat-scroll: Implement selector:top #722

merged 2 commits into from
May 21, 2020

Conversation

thet
Copy link
Member

@thet thet commented May 19, 2020

Ref: #721

Note:
IIRC, The original issue appeared when injecting content into a site where we want to scroll to the top of the page and not keep the old scroll position.
This problem is actually widely known - e.g. vue-router has a special scrollBehavior option for route-changes for this: https://router.vuejs.org/guide/advanced/scroll-behavior.html
An alternative solution would be to implement the scrolling behavior in pat-inject after the injection was done.
@cornae @pilz

cornae and others added 2 commits May 19, 2020 11:29
pat-scroll: Implement new special `selector:top` attribute value to scroll the scroll container just to the top of the page.
Ref: #721
Fixes: #721
@pilz
Copy link
Member

pilz commented May 19, 2020

@thet we have some scroll impl in inject already, did you know https://github.com/Patternslib/Patterns/blob/master/src/pat/inject/inject.js#L501 ?

@thet
Copy link
Member Author

thet commented May 20, 2020

@pilz - very nice! Did not know this. Can scroll-top for pat-inject be the default? If so, then this work here might be obsolete - although it's still handy to have the option to just scroll up without any further config.

@thet thet removed the hold merge label May 20, 2020
@cornae
Copy link
Member

cornae commented May 21, 2020

@thet scroll: top as default for pat-inject would have unoverseable consequences. But… we could reach the same effect once we have global pat-inject. #703 With global pat-inject you would only have to put pat-inject on the body tag with the scroll property set to top.

What pat inject scroll cannot do is animate the scroll and sometimes that's more elegant. The top value is either way still very welcome for pat-scroll.

@pilz pilz merged commit 0c45c39 into master May 21, 2020
@pilz pilz deleted the scroll-selector-top branch May 21, 2020 18:53
thet added a commit that referenced this pull request May 21, 2020
Hotfix for #721, PR: #722
pat-scroll: Implement new special `selector:top` attribute value to scroll the scroll container just to the top of the page.
Ref: #721
Fixes: #721
thet added a commit that referenced this pull request Jul 1, 2020
Hotfix for #721, PR: #722
pat-scroll: Implement new special `selector:top` attribute value to scroll the scroll container just to the top of the page.
Ref: #721
Fixes: #721
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