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

scroll_into_view operation #102

Merged
merged 3 commits into from
Jan 24, 2021
Merged

scroll_into_view operation #102

merged 3 commits into from
Jan 24, 2021

Conversation

leastbad
Copy link
Contributor

@leastbad leastbad commented Jan 22, 2021

This PR proposes to add an operation which wraps https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView

One can do my favorite scroll operation:

cable_ready.scroll_into_view(behavior: "smooth", block: "center").broadcast

@marcoroth
Copy link
Member

Awesome! I had the same idea on my todo list 😉

I was wondering if scroll_to would fit better in this context.

@leastbad
Copy link
Contributor Author

leastbad commented Jan 23, 2021

I was wondering if scroll_to would fit better in this context.

Maybe.

I have this idea that I would release a tutorial for a scroll_to custom operation that would support a lot more stuff:

  • you can jump to a specific y pixel distance from the top
  • you can control the scroll speed
  • you can provide an easing function to the scroll
  • you can provide a top-offset to respect top navigation bars when scrolling up

My main beef with scrollIntoView is that you can't "scroll to x", you can only "scroll at id".

@hopsoft
Copy link
Contributor

hopsoft commented Jan 23, 2021

Thank you for all of these proposals for new operations. I love it.

One convention that CableReady has adhered to "fairly well" that I'd like to formalize into a best practice is to ensure that operation names match as closely to the formal DOM API specification as possible. For example, if we use scrollIntoView in the internals, then the CR method should be scroll_into_view etc... I think that this naming convention provides the library with a few advantages.

  1. Immediate grokability for people already familiar with the DOM
  2. Principle of least surprise as to what the operation does and how it behaves
  3. Retains maximum flexibility for extending the library with more operations (the hard work of naming things has generally been done for us)

@leastbad
Copy link
Contributor Author

That makes great sense, but there are important exceptions - I'm talking about graft.

https://indepth.dev/posts/1161/here-is-why-appendchild-moves-a-dom-node-between-parents

First, I hold up this article as evidence that most devs have no idea that they can use appendChild to graft an element and keep their Stimulus controllers intact. The primary use case in 2021 is not the same as it was when it was added to the API in the 90s.

Second, naming it append_child isn't great because it's not attempting to be a true wrapper for that function; appendChild has functionality for creating elements with supplied HTML and we don't even try to do that.

Third, I honestly believe that graft is both instructive and marketable. It's also shorter. 😸

@hopsoft
Copy link
Contributor

hopsoft commented Jan 23, 2021

there are important exceptions

I completely understand, which is why there's a proviso to violate the best practice for scenarios that don't map cleanly.

Having said that, I'm not sure graft fully communicates what's happening in that operation. Namely, that you're moving an existing element from one parent to another. Perhaps something like reparent more effectively communicates what's actually happening... granted it's not quite as punchy of a name.

@leastbad
Copy link
Contributor Author

image

More than just an awesome name.

@leastbad leastbad changed the title scroll operation scroll_into_view operation Jan 24, 2021
javascript/cable_ready.js Show resolved Hide resolved
@leastbad leastbad merged commit d73ba04 into stimulusreflex:master Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants