Added support for legacy Imulus.TableEditor formatted data

[hfloyd]

Resolves https://github.com/limbo-works/Limbo.Umbraco.Tables/issues/28

[abjerner]

Thanks @hfloyd 👍

Could you share your original JSON? You shared a partial example here, but I'd like to see the entire JSON structure. Feel free to blank out any sensitive information.

That would hopefully allow me to better test your PR 😉

[hfloyd]

LegacyImulusTableEditorExampleData.txt

Sure! see attached

[abjerner]

Thanks 👍

Looking at the legacy format, I suspect we might have based ours on Imulus.TableEditor.

Anyways - I suppose we can use the same parsing logic for either format, as looking at only the cells array should be sufficient (I think). rows/rowStylesSelected and columns/columnStylesSelected shouldn't matter.

[hfloyd]

I wasn't sure if there was a specific reason that the empty rows/columns were in the JSON, so I figured I wouldn't mess with the existing functionality, just add alternate functionality in case "rows" didn't exist.

[hfloyd]

@abjerner Do you need me to do anything else in the code for this?

[abjerner]

@hfloyd no, I think the PR is fine. I just haven't had the time to look further into it yet 😦

Like mentioned in my previous comment, there may be some room for improvement so the parsing logic is the same for both the old and the new format. Whether you want to look a bit into that is up to you - otherwise I'll do so when I get around to reviewing your PR 😉

[hfloyd]

Ok, I just wanted to make sure you weren't waiting on anything from me. 🙂

[hfloyd]

I removed the "checking" of which data format might be in use, and just have the "maxCell" value used for the index, and it seems to work fine for old and new data now.

[abjerner]

Hi @hfloyd and again thanks for your contributions 👍

I had hoped to look at your PR sooner, but other work have ended up getting in the way.

Following your last commit, I had another look, and I'm most likely going to reject this PR. Not because it isn't good, but because our own parsing logic was a bit of a mess. By starting over, I've been able to improve the code a lot - e.g. by avoiding LINQ and unnecessary iterations, and hopefully also making the code a bit cleaner. I hope that makes sense 😉

It's getting late here, so I'm stopping for today. I haven't committed anything yet, but I'll try to find some time to wrap this up tomorrow. I'll do some more testing, and hopefully I will be able to push a new release then 😎

[hfloyd]

Hi @abjerner! Ok, cool. I just needed something functional for my current project, so I'm using my own build right now (in other words - no big rush needed).

Incidentally, I figured out that though the legacy data format allowed the data to appear in the UI, and you could click on an existing cell to edit it, the add/remove columns and rows functionality wasn't working (I guess that's what those collections of "{}" were providing markers for), so I have begun updating the legacy data with some simple "find/replace" logic to bring it more in line with the new format.

[abjerner]

@hfloyd you can see the updated C# code in the v13/dev/imulus branch.

The updated implementation only looks at the cells property, and completely ignores the rows/rowStylesSelected and columns/columnStylesSelected.

If the Angular implementation in the backoffice still relies on rows and columns, I guess we can do something similar there as well.

[abjerner]

@hfloyd you can see the C# fix in this commit: https://github.com/limbo-works/Limbo.Umbraco.Tables/commit/e5f168124a35791efaf12e8c97db68a7280a2577

I also had a quick look at the Angular part. It seems that if columns and rows are missing, the easiest approach is to create them based on the cells array. This is fixed in: https://github.com/limbo-works/Limbo.Umbraco.Tables/commit/76012e3f3c6ae57f5594bf76376f49862c3342df

Could you test whether this works with your data?

Ideally we shouldn't need the columns and rows arrays, but removing this entirely from the Angular logic would be a bigger step, so I've instead opted for the solution above for now.

---

On another note, I can see that the example data you posted earlier has 25 columns, but I can see that our addColumn function will return right away if the table has exceeded 12 columns. I'm not entirely sure why we added this check/limitation, but I guess it's to prevent a bad UI experience. Do you have any thoughts on this? Recreating your example data would currently not be possible through the UI due to this limitation.

[hfloyd]

Thanks, @abjerner, for the updates.

I pulled, built a local NuGet package and installed it, but now my site won't build with weird errors ("The type or namespace name 'Tables' does not exist in the namespace 'Limbo.Umbraco' (are you missing an assembly reference?)") Once I figure out what happened there, maybe I can tell you if it works 😝

Regarding the column limits... I'm sure it IS a crappy user experience... but this is a financial firm, and they are using the table data sometimes to render the tables, and sometimes as a datasource for charts. I'm sure there is a better architecture for these use-cases, but I'd need to discuss it, implement it, and then transition all the current data... so for the time being (that is, trying to launch a version-updated site), I will just have to deal with it. Ideally the property editor would allow the data, regardless of usability... perhaps showing a warning message if it exceeds a certain size? I don't know. I noticed even on smaller tables, the width of the screen will cause horizontal scrolling, and if you have a table with 12 columns, but they have long un-breaking content (like filenames or who knows what), it will still make sideways scrolling necessary.

I did notice that if you have a bunch of columns, the little trash can icons often don't align properly with the associated column, which could also get problematic, from a user perspective. There are ways to solve for this (perhaps showing light grey column numbers with each trash can and each first row column of actual content, so even if they aren't aligning, they can be associated?)

Looking at the Imulus v7 one, its UI is a little different - with a more "excel-like" layout:

(from: https://github.com/xorcery/TableEditor?tab=readme-ov-file#umbraco-7-tableeditor)

I'm sure horizontal scrolling was still an issue, but the row/column outlines might make it easier to keep one's place.

[hfloyd]

Ah, the issue was that this is targeting v13/.Net 8 and my site is v10/.Net 6. I changed it to .Net6, and updated the project file with <PackageReference Include="Umbraco.Cms.Core" Version="[10.0.0,14.0.0]" /> etc., rebuilt the package, and installed it with better results. 😅

The UI now works whether the underlying data is in the new or old format! ✔

[abjerner]

Ah, the issue was that this is targeting v13/.Net 8 and my site is v10/.Net 6. Great you found out. I was testing with a local NuGet package, and that worked fine.

Regarding the column limits... I didn't know we had that limit, so I may look into removing it. The UI could probably also do with an improvement, but that seems like a bigger task.

The UI now works whether the underlying data is in the new or old format! Awesome - I'll look into pushing a new release.

[hfloyd]

FYI - I integrated your updates into v1 for pre-Umbraco-13 usage. https://github.com/limbo-works/Limbo.Umbraco.Tables/pull/34