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

Add "scroll" prop to <A> #2666

Closed
paul-hansen opened this issue Jul 3, 2024 · 5 comments
Closed

Add "scroll" prop to <A> #2666

paul-hansen opened this issue Jul 3, 2024 · 5 comments
Labels
documentation Improvements or additions to documentation

Comments

@paul-hansen
Copy link
Contributor

Is your feature request related to a problem? Please describe.
I would like to have a table that has pagination buttons that are proper links and set the ?page=X query parameter. Unfortunately this causes it to scroll to the top of the page when clicked. Because the table isn't at the top of the page, it's jarring and annoying to have to scroll down to the table again every time.

Describe the solution you'd like
I'd like to be able to pass scroll=false to Leptos' <A> component to tell it not to scroll to the top of the page when navigating.

Describe alternatives you've considered
I've considered just using on:click and use_navigate but that's discouraged because it isn't as accessible according to the Leptos book:

You should almost never do something like <button on:click=move |_| navigate(/* ... */)>. Any on:click that navigates should be an <a>, for reasons of accessibility.

Additional context
I looked up how this was done in the React world for inspiration and here they have a scroll property on their <link> component: https://nextjs.org/docs/pages/api-reference/components/link#scroll

@paul-hansen
Copy link
Contributor Author

paul-hansen commented Jul 3, 2024

I was looking into implementing this and it looks like it does detect a noscroll attribute already.

scroll: !a.has_attribute("noscroll"),

We probably should either document this somewhere or make a prop on <A> that sets it for us for more visibility. Happy to help with this, whichever is wanted.

@gbj gbj added the documentation Improvements or additions to documentation label Jul 3, 2024
@paul-hansen
Copy link
Contributor Author

Looks like the noscroll attribute is missing in 0.7
image

@gbj
Copy link
Collaborator

gbj commented Dec 6, 2024

Ah. Probably because it's not an actual HTML attribute, and we do check those now.

I'm open to ideas. One easy one would be to support data-noscroll as an additional possibility, which you definitely can add.

@paul-hansen
Copy link
Contributor Author

paul-hansen commented Dec 6, 2024

Yeah that makes sense, either that or take a component prop. Component prop would be more self documenting, but probably not worth much extra effort if a data attr is easier.

For my project I think I need to hold off on upgrading for a bit, so no rush from me. I've got some custom components I need to rework which will make upgrading easier in the future. I can work on a PR for noscroll eventually if no one beats me to it.

gbj added a commit that referenced this issue Dec 7, 2024
@gbj
Copy link
Collaborator

gbj commented Dec 7, 2024

I think I added a scroll prop straightforwardly enough in #3333 but I'll admit I didn't really test it — want to try it out to see if it's suitable?

@gbj gbj closed this as completed in 49366be Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants