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

Regression from PR 240 around removing target children before append / prepend? #928

Closed
nickjj opened this issue May 29, 2023 · 4 comments
Closed

Comments

@nickjj
Copy link

nickjj commented May 29, 2023

Hi,

PR #240 avoids duplicates by removing the element before appending or prepending it but this might prevent wanting to have a workflow like:

  • User creates a comment
    • We can classify this user as the comment author
  • The comment author's comment gets a highlight class added to it
    • This provides nice UI feedback that something happened
  • The comment gets broadcast to all listeners without the highlight class

Here's some of the code that you might use to put this together. I'm extracting this example out of one of my apps and renamed a few things so apologies if the syntax is off.

create.turbo_stream.erb
<%= turbo_stream.append @comment %>
<%= turbo_stream_action_tag :highlight, target: @comment, class: "border-l-4 border-sky-300 bg-sky-50" %>
application.js
StreamActions.highlight = function() {
  this.targetElements.forEach((element) => element.className += this.getAttribute("class"))
}

Then in your model you can broadcast this and everything works in the sense that the comment author sees their new comment and all listeners see the new comment. However the problem is the comment author's highlight isn't there because I think it's due to PR #240 deleting the element with the highlight and then the broadcasted comment gets appended without the highlight.

I'm not 100% sure if this is the case but I do know for sure that when I create a comment without broadcasting from my model then the highlight appears and if I comment while broadcasting from my model then the highlight is never visibly shown.

Is there another pattern or way to handle the above use case? Thanks!

@dhh
Copy link
Member

dhh commented Jun 18, 2023

I'd hook this up with a stimulus controller that does the checking of whether the author is the current user against a meta tag in the head that includes the current user id. That's how we do a bunch of "when the author is X, then.." work at 37s.

@dhh dhh closed this as completed Jun 18, 2023
@nickjj
Copy link
Author

nickjj commented Jun 18, 2023

Thanks. Would you be opening to helping piece this together? I'd be happy to turn it into a personal blog post tutorial or even an example in the official docs afterwards.

I'm not a Hey member but I did sign up for a trial to see how to piece everything together. I used your source code to get the contents of what's listed below.

Here's what I did so far:

app/views/ layouts/application.html.erb
    <meta name="current-identity-id" content="<%= Current.user.id %>">
app/javascript/helpers/current_helpers.js
export const current = new Proxy({}, {
  get(target, propertyName) {
    const result = {}
    const prefix = `current-${propertyName}-`

    for (const { name, content } of document.head.querySelectorAll(`meta[name^=${prefix}]`)) {
      const key = camelize(name.slice(prefix.length))
      result[key] = content
    }

    return result
  }
})

function camelize(string) {
  return string.replace(/(?:[_-])([a-z0-9])/g, (_, char) => char.toUpperCase())
}
app/javascript/helpers/index.js
export * from "./current_helpers"
app/javascript/controllers/highlighter_controller.js

I'm not at the point of making this fully work yet but current is always {} as well.

import { Controller } from '@hotwired/stimulus'
import { current } from "../helpers"

export default class extends Controller {
  connect() {
    // TODO: This is an empty Proxy object
    console.log(current, current.id)
    // The fix is to reference `current.identity` not `current` (see my next GitHub comment)
}

This is what I get back when I console.log things in the above controller:

Proxy(Object) {}[[Handler]]: Objectget: ƒ get(target, propertyName)[[Prototype]]: Object[[Target]]: Object[[Prototype]]: Object[[IsRevoked]]: false

In your journal_params_controller you imported current and accessed current.journal.params in the same way I imported current and accessed current.id in the above controller. What am I missing to allow this object to get populated?

I did notice in js/initializers/mailto_protocol_handler you had a number of things related to awaitCurrentIdentity but I wasn't sure how that would apply back to this use case.

@nickjj
Copy link
Author

nickjj commented Jun 18, 2023

Ah hah, I figured out the issue that led to where current was {}. I needed to reference current.identity not current. The comment in current_helpers.js showed a reference. After making that 1 change it produced the user's ID in the above console log with no other edits.

Hooking in this controller to execute only for the current user without depending on create.turbo_stream.erb is a different can of worms tho.

@nickjj
Copy link
Author

nickjj commented Jul 13, 2023

I'm curious how other folks handle this.

We have the ability to create custom StreamActions, which allow us to do something like this:

StreamActions.remove_later = function() {
  this.targetElements.forEach((element) => element.className += this.getAttribute("class"))

  setTimeout(() => {
    this.targetElements.forEach((element) => element.remove())
  }, this.getAttribute("after"))
}

And then in a destroy.turbo_stream.erb we can do:

<%= turbo_stream_action_tag :remove_later, target: dom_id(@answer), after: 500, class: "transition-opacity duration-500 ease-in opacity-0" %>

Then when an element gets deleted with Turbo Streams you have a very nice fade out effect and the DOM element gets removed.

But as soon as you use any broadcast_*_to method to broadcast the deleted element to anyone but the person deleting it, you lose this effect or more generally the ability to use custom stream actions due to the reasons described in this issue.

@dhh I hate to at-mention you directly, but what patterns can we use to leverage custom stream actions while broadcasting? Do you have any available code snippets for this? Custom stream actions feel like a great way to quickly perform custom client side work after a broadcast but with the way things are currently implemented, they don't fire.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants