olifolkerd / tabulator

Interactive Tables and Data Grids for JavaScript
http://tabulator.info
MIT License
6.71k stars 818 forks source link

Editable collapsed data #3749

Open lekoala opened 2 years ago

lekoala commented 2 years ago

*Is your feature request related to a problem? Please describe.** I'm working on an editable table with a couple of editable fields. I'd like this to work with the collapsed responsive mode. But any column with an editor is not editable because of the way the collapsed table is generated. Additionally, if data is updated through the editor (let's say column A updates a value in cell B) collapsed data is not refreshed.

Describe the solution you'd like It would be great if there was a way to clone the cell editor in the collapsed view. Effectively, editing through the collapsed view or through the regular view should have the same effect.

Describe alternatives you've considered I had a look at the responsiveLayoutCollapseFormatter but that seems too limited to do this properly since you don't get the actual cell.

If you are interested and have some ideas on how to achieve this i'd be happy to work on it

lekoala commented 2 years ago

https://github.com/olifolkerd/tabulator/pull/3755 this is to fix the generated data not being updated after edit

lekoala commented 2 years ago

@olifolkerd so I have a working proof of concept. here is how it works. I introduce a new collapse mode called "flexCollapse". in this mode, there is no generated data. Instead, columns are displayed based on the open state as full width rows. I make sure they are at the end of the "row" by using the "order" attribute and setting the row to display:flex So this display all the "hidden" rows neatly below the row when open. But I need the label, so I add a data-attribute a with a little bit of css, I display that label alongside the row. Upon editing, I hide the label so that the input can take the full width a behave exactly like it should.

I introduced a new event for the responsiveCollapse formatter called row-responsive-toggled. Actually, I think that the formatter should only fire that event and let the module handle what should happen (by default, showing/hiding the collapsed el)

Hopefully you find my idea interesting. I don't quite see how I could do this without changing how the core works. If you don't like the idea or find it too hacky or something, I'd like to work out how I can implement my solution without editing core files, as a plugin.

olifolkerd commented 2 years ago

do you have a JS fiddle that demonstrates this? :)

lekoala commented 2 years ago

yes i had to work a bit to make it work ;-) https://jsfiddle.net/5vnu6901/5/ it doesn't include the editor, but you will see the general idea there

you can check the changes in my patch-5 branch https://github.com/lekoala/tabulator/tree/patch-5

lekoala commented 2 years ago

updated the fiddle with editors and highlighted cells https://jsfiddle.net/c8gp059z/ (sorry it's a public fiddle so i had to change the link)

lekoala commented 2 years ago

@olifolkerd what do you think of the latest jsfiddle ? Does it seem like something you would consider to include? The general approach is still a bit rough, but I think I've demonstrated its benefit (using actual columns, no need for generating responsive data...) and it is the only (easy) way to make editors work for responsive columns Or if you can find a way to abstract that logic so that it could be implemented in an external plugin that works for me too.

olifolkerd commented 2 years ago

I havnt had a chance to look into this one yet, I am planning to have a Tabulator day next weekend and will catch up with it then.

Cheers

lekoala commented 2 years ago

@olifolkerd with the work on 5.3 being done, what do you think about this ? it seems safe to introduce (since it's a new responsive mode without side effects) if you are open to this approach

olifolkerd commented 2 years ago

Hey @lekoala

Thanks for sending over a link to that fiddle, There are definitely some good bits in there it would be worth pursuing.

I like the concept of editable cells in the responsive collapse view. and the editors in your example seem to work well.

I would not be averse to there being a collapse mode and a collapseEditable mode to allow users the choice between editable and non editable data, but im not sure why a whole new flex layout approach would be needed to achieve this or what a flexCollapse option adds to the user experience. Where possible the two should be based along the same fundamental building blocks as the collapse layout so it is easy for users to switch between the two.

There is no need for a completely different structure to the existing table layout to achieve this. I have concerns with the flex approach taken in that example, by putting labels in an before psuedo element, you make the values themselves unresponsive. Everything should exist as actual elements in the DOM to give complete control over layout and user interaction with it.

The existing collapse layout mode would only need a few tweaks to bring this functionality to it. essentially using the same approach to the editing process that you currently have. Removing the label when editing creates a loss of context that can make it confusing if you come back to a form part way through editing so that would need to remain during edit.

In your fiddle you appear to apply extra classes to the editable cell. In general I try and keep the number of classes to a minimum, in this case because the cell exists in a tabulator-responsive-collapse div, you could use that to apply the needed styling without additional classes.

I would also avoid styling the editable cells as you have in that example, Tabulator does not by default style editable cells differently, that should be left up to the user to implement if they want based on the tabulator-editable class on the cell

If that makes sense i would certainly be interested in a PR to add this functionality to the 5.4 release :)

Cheers

Oli :)

lekoala commented 2 years ago

@olifolkerd the way i see it, is that with my approach, there is no need to generate collapsed data and that simplify everything. At the moment, collapsed data is just one "blob" of things where you cannot apply specific behavior to each cell (=> not editable).

The flex approach allows to "push" everything that is responsively hidden at the end of the row by using the order css attribute. This is why I need a distinct class at the moment to enable flex behavior on the row.

If you see a way to do something similar (ie: not generate the responsive content to keep original column behaviour on responsively hidden columns) within the current way of doing things, I'm all for it, but I have no idea how to do that without massive changes :-)

olifolkerd commented 2 years ago

@lekoala I think there are advantages but the use of before is defo a non starter, that would need to be actual elements for this to work correctly.

I would also have concerns that the flex approach would break any content added by someones row formatter because it is not self contained.

There would be nothing to stop you moving the cell elements into/outof another containing element below the row. but you will need something tabular to allow full scaling of the row titles and maintaining alignment across rows

Is the source for your collapse function anywhere? i am happy to offer some pointers.

Cheers

Oli :)

lekoala commented 2 years ago

i'm not sure what you mean by "the use of before is defo a non starter"? Which "before"?

i also don't see why the flex approach would make them not self contained ? the only part i'm not really happy with are the pseudo elements used to display the labels: it works but it's kind of hacky.

as for the implementation, you can check out these two files https://github.com/lekoala/silverstripe-tabulator/blob/master/client/src/modules/ResponsiveLayout/ResponsiveLayout.js https://github.com/lekoala/silverstripe-tabulator/blob/master/client/src/modules/Format/defaults/formatters/responsiveCollapse.js

i introduced a 'row-responsive-toggled' event that is triggered by the formatter (i think it's a good improvement to avoid adding logic in the formatter, and delegate the toggling functionality to the responsive layout module itself)

there is a "toggleFlexRow" function that takes care of setting the required css styles

so even if it has limitations, this new mode has some real advantages. it adds very little code to the codebase and you can choose to not support some use cases (eg: very specific formatters) anyway. The current "collapse" mode already does this by not supporting editable cells.

i can make a PR if that makes things any clearer

olifolkerd commented 2 years ago

Hey @lekoala

Sorry for the confusion there, i meat the before psudo elements you are using for the labels, but you have identified that your self in your next sentence there.

Which is why i was talking about keeping things in a table, because you dont want to be fixing the width of the labels arbitrarily and you need to be keeping the width of the label column the same across all labels to keep things in line, which a table will do for you.

A PR would certainly make things easier as at the moment im having to go off what i can see in the DOM and some guesswork.

we can then move the discussion to there.

Cheers

Oli :)

lekoala commented 2 years ago

ok you can check here

https://github.com/olifolkerd/tabulator/pull/3875/files

to see the required changes in the current state of things (which is: flex mode, and some additional css)

i fully agree with you that the pseudo elements for labels is not great, but it does make things much much simpler. it does have the limitation of setting an arbitrary width, and at the same time, since we have a full width row and a small screen, having a set size is not completely crazy. a table would do things for me, but that also means i need to create/manage it ;-)

drypatrick commented 7 months ago

ok you can check here

https://github.com/olifolkerd/tabulator/pull/3875/files

to see the required changes in the current state of things (which is: flex mode, and some additional css)

i fully agree with you that the pseudo elements for labels is not great, but it does make things much much simpler. it does have the limitation of setting an arbitrary width, and at the same time, since we have a full width row and a small screen, having a set size is not completely crazy. a table would do things for me, but that also means i need to create/manage it ;-)

I would like to have this feature in future releases