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

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

Open
hypno2000 opened this issue Sep 28, 2024 · 13 comments · May be fixed by #1465
Open

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

hypno2000 opened this issue Sep 28, 2024 · 13 comments · May be fixed by #1465
Assignees
Labels
bug Something isn't working v0.4
Milestone

Comments

@hypno2000
Copy link

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

3c4ddc1

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

@hypno2000 hypno2000 added the bug Something isn't working label Sep 28, 2024
@bcardarella
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

bcardarella commented Sep 28, 2024

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
Copy link
Collaborator

bcardarella commented Sep 28, 2024

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
Copy link
Author

hypno2000 commented Sep 28, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Contributor

carson-katri commented Oct 2, 2024

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
Copy link
Collaborator

@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.

@carson-katri carson-katri linked a pull request Oct 3, 2024 that will close this issue
@bcardarella bcardarella added this to the 0.5.0 milestone Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v0.4
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants