IDEMSInternational / open-app-builder

PLH App Frontend
GNU General Public License v3.0
5 stars 24 forks source link

feat: add data-items eval context fields #2220

Closed chrismclarke closed 3 months ago

chrismclarke commented 4 months ago

PR Checklist

Description

Update items and data_items template row types to add support for metadata @item._first and @item._last to more easily determine if an item is the first or last rendered row (after potential filter operations).

Author Notes

Dev Notes

Git Issues

Closes #

Screenshots/Videos

Updated component_data_items sheet

Screenshot 2024-03-12 at 11 55 26 Screenshot 2024-03-12 at 11 55 59
jfmcquade commented 4 months ago

Thanks, very helpful.

See my update/fix in these commits (the _index should be relative to the rows of the data list, not the rows of the rendered template).

I haven't tested extensively but I have merged into my branch for #2215 and it has been helpful. I'm still working on that PR but will update soon.

chrismclarke commented 4 months ago

See my update/fix in these commits (the _index should be relative to the rows of the data list, not the rows of the rendered template).

I don't quite follow what your changes do.

Previously the lastRowIndex was calculated by

const lastRowIndex = rows.length - 1;

Now the total rows is calculated by

 const rowIds = Object.values(dataList).map((v) => v.id);
 const lastRowIndex = rowIds.length - 1;

But given that the dataList has just been mapped to ids the length will remain unchanged, so I don't think that change achieves anything.

So then the next (and I'm assuming more essential part) of the code which I'm struggling to follow/understand also... Before _first and _last properties are calculated based on the available data (as passed to the items following any additional sort/filter ops). But you've changed to instead lookup the item in the original data list (?)... surely in doing so we lose all positioning following the filter/sort ops

E.g. I've updated the demo to include a 3rd item and filtering data based on completed column. With your updates then we lose first/last as completed items are toggled we get some weird first/last states not matching up with the data

image

So my only guess is we're trying to address different problems. Could you please update the example sheet to make it clearer what this should achieve (and why it fails with the original implementation)?

jfmcquade commented 4 months ago

So my only guess is we're trying to address different problems. Could you please update the example sheet to make it clearer what this should achieve (and why it fails with the original implementation)?

I think the issue is that we shouldn't be looping over rows to assign these metadata fields, but instead should be looping over the filtered data list (which is not the same). Although my changes don't properly fix the issue...

The rows array contains all rows to be rendered in the data items block, which don't necessarily map 1-to-1 onto our list of items.

I've pulled the example into its own temporary debug sheet here to get clearer console logs, adding in some static content rows within the data items loop.

Screenshot 2024-03-07 at 09 41 50

Here's the output using the code before I made my changes:

Screenshot 2024-03-07 at 09 41 40 Screenshot 2024-03-07 at 09 41 24

In this case we're comparing each individual template row to the array of all rows. This array has a length of 6, whereas we're only rendering content associated with 2 items. The last item gets assigned true for _last for the because the last template row in the items loop is associated with that item. And in the simpler example without the additional static rows, the first item was being assigned true for _first, but these values will only happen to be correct if the template is set up a certain way.

I tried to fix this by referring to the dataList rather than the rows, but I wrongly assumed that the dataList passed to the setItemMeta() method was already filtered. I think we need to access the filtered data within the component, and loop over this to set the item metadata fields (the action metadata should still loop over the rows I believe).

chrismclarke commented 4 months ago

Ah - I finally get it! Thanks, quite a few layers of abstraction to work through (highlights that could also be quite nice to include spec files for components that have complex behaviour, or moving the code to spec file that can be more easily tested)

To address the issue I've done the following in b042a1f:

I think now things should finally be working. See updated demo below, where _first and _last update and are both enabled as true for items lists that are filtered to single entries

Screenity video - Mar 7, 2024.webm