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

Editable collapsed data #3749

Open
lekoala opened this issue May 16, 2022 · 15 comments
Open

Editable collapsed data #3749

lekoala opened this issue May 16, 2022 · 15 comments
Labels
Suggested Feature A suggested feature that is waiting review

Comments

@lekoala
Copy link
Contributor

lekoala commented May 16, 2022

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 lekoala added the Suggested Feature A suggested feature that is waiting review label May 16, 2022
@lekoala
Copy link
Contributor Author

lekoala commented May 17, 2022

#3755 this is to fix the generated data not being updated after edit

@lekoala
Copy link
Contributor Author

lekoala commented May 17, 2022

@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
Copy link
Owner

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

@lekoala
Copy link
Contributor Author

lekoala commented May 17, 2022

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

lekoala commented May 20, 2022

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

lekoala commented May 23, 2022

@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
Copy link
Owner

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

lekoala commented Jul 19, 2022

@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
Copy link
Owner

olifolkerd commented Jul 24, 2022

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

lekoala commented Jul 25, 2022

@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
Copy link
Owner

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

lekoala commented Jul 25, 2022

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

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

lekoala commented Jul 25, 2022

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Suggested Feature A suggested feature that is waiting review
Projects
None yet
Development

No branches or pull requests

3 participants