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

Actions 'after' and 'before' added. Scripts enhanced for windows usage. #121

Merged
merged 3 commits into from
Jun 8, 2021

Conversation

guitarneck
Copy link
Contributor

Hello,

A simple improvment whith after and before actions. So, it give the ability to update some kind of lists.
Let says we have a <ul...>...</ul> list, and want to update it with <li...>...<li> in a desired place, after or before a given target inside the list.

By the way, I add scripts to package.json for windows users.
Hope you find it usefull.

With unit tests process (It needed a parent for this purpose). Maybe it needs additionnal checks, to insure a parent exists ?

Have a nice day !

@guitarneck
Copy link
Contributor Author

The test with fixture svg crash. It's awaiting a image/svg+xml mime type, but the file has no extension and is loaded as text/plain in fact.
should it be named svg.svg, and so tests adapted ?
It fails on line 48, not in 44. Weird.

@@ -1,10 +1,18 @@
import { StreamElement } from "../../elements/stream_element"

export const StreamActions: { [action: string]: (this: StreamElement) => void } = {
after() {
this.targetElement?.parentElement?.insertBefore(this.templateContent,this.targetElement.nextSibling)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think nextSibling could be undefined when there is only one element present. I suggest using the insertAdjacentHTML API for both actions. https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentHTML

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
It can be null with insertBefore, this is well handle.
https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore

If this is null, then newNode is inserted at the end of parentNode's child nodes.

insertAdjacentHTML seems not to be a good choice. Templates content are DocumentFragment, so insertAdjacentElement could be a better choice. But it's less compliant with all browser.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I didn't know about that null detail.

@qinmingyuan
Copy link

Want this feature so much~

@seanpdoyle seanpdoyle added the enhancement New feature or request label Apr 1, 2021
@dhh dhh merged commit d7884a7 into hotwired:main Jun 8, 2021
@itsliamegan
Copy link

I realize this was just merged, but are the names of the actions set in stone? Whereas all the other turbo stream actions are verbs (replace, append, etc.) these two are adjectives and they feel out of place. Perhaps "insert" as the action name and an additional attribute like "direction" to specify before/after?

balvig added a commit to balvig/turbo that referenced this pull request Jun 10, 2021
Removes the risk of accidentally committing the lock file, as in hotwired#121

I might be missing a reason it wasn't in .gitignore already though! 🙇 
Is there a use case I'm overlooking?
@dhh
Copy link
Member

dhh commented Jun 10, 2021

@itsliamegan I was thinking about that too, but this should be read in context, so it's action="before" target="element", and then I think it reads fine as "before target element, THIS". Which isn't too different from action="append" target="element", which also needs a little finessing in your head to read properly as "append TO target element, THIS".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

6 participants