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 append, prepend and replace operations #90

Merged

Conversation

marcoroth
Copy link
Member

@marcoroth marcoroth commented Dec 27, 2020

Type of PR

Enhancement

Description

Adds three new operation to CableReady. After using jQuery for many years and never remembering the spelling of insertAdjacentHTML and the option it takes I thought it would make sense to introduce the append, prepend and replace operations.

Why should this be added

Coming from jQuery or even native JS the terms and methods append, prepend and replace are easier to remember and could be more familiar in my opinion.

Also see:

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

@marcoroth marcoroth force-pushed the append-prepend-replace-operations branch from 1c33cf2 to 386a0ef Compare December 27, 2020 04:50
@marcoroth marcoroth changed the title Add append, prepend and replaceWith operation Add append, prepend and replaceWith operations Dec 27, 2020
@marcoroth
Copy link
Member Author

I thought about calling it just replace, but since we always use the same name as the JavaScript method I kept it replaceWith.

@marcoroth marcoroth force-pushed the append-prepend-replace-operations branch 2 times, most recently from 95c503d to d0681b6 Compare December 27, 2020 05:23
@marcoroth marcoroth force-pushed the append-prepend-replace-operations branch from d0681b6 to 0e4e2b6 Compare December 27, 2020 05:25
@hopsoft
Copy link
Contributor

hopsoft commented Dec 27, 2020

Agree with keeping the same names provided by the DOM. I'm good to add these methods given that they all accept a DOMString as well as a Node.

Copy link
Contributor

@hopsoft hopsoft left a comment

Choose a reason for hiding this comment

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

This should work by passing a DOMString, so there isn't a requirement to construct a Node.

@marcoroth
Copy link
Member Author

marcoroth commented Dec 27, 2020

@hopsoft I tried that. But it always just added the HTML as actual text (that's the difference to insertAdjacentHTML).
So you can't just pass a HTML String, unless you can somehow convert a regular String to a DOMString.

@hopsoft
Copy link
Contributor

hopsoft commented Dec 27, 2020

In that case I propose that we use template rather than div, similar to what we're doing on morph.

@hopsoft
Copy link
Contributor

hopsoft commented Dec 27, 2020

The next question is whether or not there's a material difference in the following methods.

  • append, prepend vs insertAdjacentHTML
  • replaceWith vs outerHTML

I'm mostly curious... as I think this PR is a good addition for developer happiness.

@leastbad
Copy link
Contributor

lol... there's an echo in here

@marcoroth
Copy link
Member Author

There isn't really a difference from a user perspective other than the method names.

I first wanted to implement these operations with method aliasing, but then I realized that the DOM even offers such methods. Since we always used the DOM method corresponding to the CableReady operation I thought I will do the same here.

Also: If we would go the alias way, we have the problem with the events. If you alias it in the backend then the event in the frontend wouldn't match anymore and if you alias it in the frontend then both the method and the alias method events would get triggered.

That's why I went with the DOM methods in this implementation in the first place.

Regarding the differences: This answer here summarized it pretty good in my opinion: https://stackoverflow.com/a/16127049/5052329

Since we always just have a HTML string we need first to construct a Node in order to use append, prepend and replaceWith methods. This is probably also the reason why we just have insertAdjacentHTML, innerHTML and outerHTML now, because they just take a HTML String and convert it to a Node on their own.

But if you have the feeling that we should go the "Alias-Way" we can swap out the JS implementation. So we should just call insertAdjacentHTML('beforebegin', html) in the frontend append implementation. But I don't know if this is even more confusing tbh.

@hopsoft
Copy link
Contributor

hopsoft commented Dec 27, 2020

Ah ok. Does that mean we could the simplify the implementation to something like this then?

  append: detail => {
    activeElement = document.activeElement
    const { element, html, focusSelector } = detail
    dispatch(element, 'cable-ready:before-append', detail)
    element.insertAdjacentHTML('beforeend', html)
    assignFocus(focusSelector)
    dispatch(element, 'cable-ready:after-append', detail)
},

// etc...

@hopsoft
Copy link
Contributor

hopsoft commented Dec 27, 2020

Also, for the record.

I realized that the DOM offers such methods [append, prepend, replaceWith]

👆🏻 This is the primary reason why I think adding these methods is a good idea.

@marcoroth
Copy link
Member Author

Ah ok. Does that mean we could the simplify the implementation to something like this then?

Exactly! If you think this approach is better I'm happy to rewrite it.

@hopsoft
Copy link
Contributor

hopsoft commented Dec 28, 2020

There doesn't appear to be a material difference, so my vote is to converge on a single internal implementation by effectively delegating to the existing methods.

@leastbad
Copy link
Contributor

How would this delegation work?

I am hoping to keep things pretty obvious.

@hopsoft
Copy link
Contributor

hopsoft commented Jan 1, 2021

#90 (comment)

@marcoroth marcoroth changed the title Add append, prepend and replaceWith operations Add append, prepend and replace operations Jan 1, 2021
…operations

Marcoroth/append prepend replace operations
@leastbad leastbad merged commit 3b3fbeb into stimulusreflex:master Jan 17, 2021
@marcoroth marcoroth deleted the append-prepend-replace-operations branch January 17, 2021 15:42
hopsoft added a commit that referenced this pull request Jan 23, 2021
* Global config, simplify threading, custom operations

* Remove unused artifact

* Remove redundant require

* GitBook: [master] one page modified

* Support chaining and rip deprecations out

* GitBook: [master] one page modified

* multiple selector element operations (#92)

* `select_all` option added to all operations which support a `selector`

* `cancel` option added to almost all operations, intended to be set in a `before-` event callback

* substantial reorganization of `cable_ready.js` to match the order in the docs

* Bump nokogiri from 1.10.10 to 1.11.1

Bumps [nokogiri](https://github.com/sparklemotion/nokogiri) from 1.10.10 to 1.11.1.
- [Release notes](https://github.com/sparklemotion/nokogiri/releases)
- [Changelog](https://github.com/sparklemotion/nokogiri/blob/master/CHANGELOG.md)
- [Commits](sparklemotion/nokogiri@v1.10.10...v1.11.1)

Signed-off-by: dependabot[bot] <support@github.com>

* Add `append`, `prepend` and `replace` operations (#90)

* Disable chaining after broadcast

Co-authored-by: leastbad <hello@leastbad.com>
Co-authored-by: leastbad <38150464+leastbad@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Marco Roth <marco.roth@intergga.ch>
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