-
-
Notifications
You must be signed in to change notification settings - Fork 428
Add noscroll option to goto, document sapper-noscroll on links #1320
Conversation
@Jayphen does this supercede the other PR? |
@antony Yes, the other can be closed. It did not actually implement the change correctly. |
c7f0b56
to
6fddea7
Compare
Can you add this to the documentation as well? https://github.com/sveltejs/sapper/tree/master/site/content/docs |
Sure, should I also document the currently undocumented noscroll attribute that can be used on anchors? |
Yes, that'd be great. Thanks! |
@benmccann Okay, I've added some docs. I copied the style of the Svelte docs for describing the type and default for each option: https://svelte.dev/docs#flip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this @Jayphen ! this is a really high quality contribution. appreciate the docs and tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR!
site/content/docs/08-link-options.md
Outdated
In certain cases, you may wish to disable this behaviour. Adding a `sapper-noscroll` attribute to a link... | ||
|
||
```html | ||
<a href='path' sapper-noscroll>Path</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'd prefer noscroll
attribute. Maybe others can weigh in?
<a href='path' sapper-noscroll>Path</a> | |
<a href='path' noscroll>Path</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukeed agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stickywhenclicky
imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not relevant to this PR — I only documented what currently exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer sapper-noscroll
which makes it clear that it's a Sapper specific thing. 'noscroll' feels like an built-in attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a poll of the maintainers offline and there was a preference for keeping noscroll namespaced. However, folks preferred sapper:noscroll
to sapper-noscroll
, so I updated this PR to rename the attribute.
I'll go ahead and merge this now. Thanks for the patience and input everyone!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also prefer sapper:noscroll
, but I'm curious, are there other namespaced attributes for html elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not currently, but that's where things are going to be moving in the future. rel="prefetch"
(which, when used on an anchor element, is a Sapper-only thing, which is confusingly named the same as a real HTML thing) is going to be renamed to some other namespaced sapper:
attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I like this idea. Much clearer!
This adds an option to
goto
-noscroll: boolean
- which allows preservation of scroll position when navigating programmatically.noscroll
already exists as an option onnavigate
, and this just passes the option through to that underlying method.I've added tests that show the default behaviour when using
goto
(scrolling to the top), and the preservation behaviour when the option is passed.This implements #584
Note: In working on this, I discovered a bug in
goto
/navigate
- the scroll position is not preserved when usingnoscroll: true
when only the search params are updated (not the pathname). I will add a separate issue for this.edit: added issue with test cases
Before submitting the PR, please make sure you do the following
npm run lint
!)Tests
npm test
oryarn test
)