liveview-native / liveview-client-swiftui

MIT License
376 stars 37 forks source link

[Bug]: phx-delete index and List with Sections #1462

Open hypno2000 opened 3 weeks ago

hypno2000 commented 3 weeks ago

What happened?

When List divided into Section's then phx-delete sends the index relative to the current Section that the item is in.

For example if i have this:

<List phx-delete="on_delete">
  <Section>
    <Text template={:header}>Section 1</Text>
    <Text template={:content}>Item 1-1</Text>
    <Text template={:content}>Item 1-2</Text>
  </Section>
  <Section>
    <Text template={:header}>Section 2</Text>
    <Text template={:content}>Item 2-1</Text>
    <Text template={:content}>Item 2-2</Text>
  </Section>
</List>

When i delete Item 2-2 then it sends %{"index" => 1} to event. It is not enough context for the server to know what to delete.

It would be nice if the id of the item is passed or allow some custom even handling with onDelete modifier

Library Version

3c4ddc18014be33051434fdc3fd285dad8cd1d7d

Xcode Version

16

Swift Version

6.0

On which device or simulator are you running into the problem?

iPhone

Target Device Operating System Version

IOS 18

Relevant log output

No response

bcardarella commented 3 weeks ago

Is there an equivalent to this in HTML? Or is the an issue that would be unique to native? If so then it may be a bigger issue to solve properly

hypno2000 commented 3 weeks ago

i dont think this list sections markup is in html but there is more control of what is sent with the events, ie <div phx-click="inc" phx-value-myvar1="val1" phx-value-myvar2="val2">

bcardarella commented 3 weeks ago

I cannot seem to find phx-delete as an actual event in LiveView anywhere. It's not defined in their CONSTANTS. This may be our own event, I'll have to follow up with @carson-katri next week on this.

But as I'm understanding the issue, because you have two or more distinct lists, the index value the event receives is ambiguous on which list it is acting upon?

hypno2000 commented 3 weeks ago

Yes all those list events are LVN specific. Nothing like that in LV.

It is a single List, but grouped into Sections (some references of this feature here and here).

My guess is that the index is counted by looking its immediate parent, that is Section.

One way to solve it is by adjusting the index counting by counting items in context of List not in context of its immidate parent. But if possible I would rather prefer to have direct control what data is sent to the event, ie phx-value-myvar1="val1".

bcardarella commented 3 weeks ago

One way to solve it is by adjusting the index counting by counting items in context of List not in context of its immidate parent.

This sounds reasonable, we should go up the tree and find the next closest List parent.

But if possible I would rather prefer to have direct control what data is sent to the event, ie phx-value-myvar1="val1"

This also seems reasonable. But I would probably advocate for phx-value-index or something like that instead as phx-value-xxxx is already used by phx- click.

hypno2000 commented 3 weeks ago

I have not worked with LV for a while but i'm not sure if phx-value-* is just for phx-click. By this docs it seems to be universal.

What ever the solution will be it would be good if the event handlers can be shared between web/mobile, or at least are as close as they can be, ie not using different names for the same thing.

bcardarella commented 3 weeks ago

I have not worked with LV for a while but i'm not sure if phx-value-* is just for phx-click. By this [docs]

that was an incomplete thought on my part, the situation I was considering is if you wanted a phx-click event on a list item and you also want phx-delete on each. You could have a use case where you want a different value for both events. Or if you are using phx-value beacuse of phx-click and you have a duplicate value in the list. The index should always have a way to guaranteee uniqueness

bcardarella commented 3 weeks ago

but beyond that, what I need to get confirmation on is if we can even use phx-delete. What we don't want to do is collide with upstream events that are added later to LiveView that are now making claim to that event name. I also don't like introducing lvn- event namespacing as that would be confusing with all the phx- events we do already support.

hypno2000 commented 3 weeks ago

if you wanted a phx-click event on a list item and you also want phx-delete. You could have a situation where you want a different value for both events.

Yes that's good point. I'm not sure how big of a practical issue it would be as you could define many attributes which some are used by click handler and some by delete handler. Small network efficiency overhead remains though.

My main point with phx-value-myvar1="val1" was the ability to control the value side of it (ie val1). Then i could set it to the item unique id (ie db primary key). Alternatively it would already be good if we just pass the id attribute of the item to the event.

bcardarella commented 3 weeks ago

yeah we'll get this fixed for sure, legit bug. I'll disucss with the team next week which approach we should take

carson-katri commented 2 weeks ago

I think we should remove the phx-delete attribute (I think we also have a phx-move attribute for reordering list elements), since they are not present in LiveView on the web.

Instead, the onDelete and onMove modifiers can be implemented. They'll need to be context-aware, like the Image-specific or Shape-specific modifiers, since they can only be applied to ForEach views. I think they would just apply to the nearest child List in LVN.

carson-katri commented 2 weeks ago

With the onDelete modifier, you would be able to write something like this:

<List>
  <Section style='onDelete(perform: event("delete"))' phx-value-section="1">
    <Text :for={i <- 1..10}><%= i %></Text>
  </Section>
  <Section style='onDelete(perform: event("delete"))' phx-value-section="2">
    <Text :for={i <- 1..10}><%= i %></Text>
  </Section>
</List>

As a payload, you'll receive something like this:

%{
  "index_set" => [2],
  "section" => "1"
}

The "index_set" key is coming from the modifier itself. I named it after the type of the value that is passed (IndexSet in Swift). But it could be renamed to something else, such as indices.

bcardarella commented 2 weeks ago

@hypno2000 we're going to go with @carson-katri's suggestion ☝️ and that means removing phx-delete an phx-move. This will happen in 0.4 as a breaking change.