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 responsive data #3875

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Editable responsive data #3875

wants to merge 4 commits into from

Conversation

lekoala
Copy link
Contributor

@lekoala lekoala commented Jul 25, 2022

This is to make discussion around #3749 easier

It is not meant to be merged as is

@olifolkerd
Copy link
Owner

olifolkerd commented Jul 28, 2022

Hey @lekoala

Thanks for submitting this as PR, it has made it much clearer to see how you have things setup.

Before i discuss the PR, i just wanted to reframe this discussion as i feel the talk on the issue this came from was starting to become unconstructive.

While i appreciate that you have spent time building this out for your usage case, when i accept a PR request, i must approach it from the perspective of the needs of entire user base, which means at times some of the changes i request will not directly apply to your usage case but will none the less be needed for this PR to be successfully included in the project.

As per my initial feedback the following changes would need to be made for this to work out correctly:

Layout

At the moment you are using psuedo before elements to build your labels using fixed widths. This presents many challenges with making a layout that is responsive to a large number of different column title configurations and would need to be changed to use actual elements that users have the control to style and done in such a way that would maintain the correct label/cell spacing across multiple lines

Now i understand that in your initial usage case that using flex layout on the row was an effective solution, and it represented a simpler option, but i need to be clear on this, that converting the row to a flex layout is not a systemically viable solution. Many other users use row formatters, and other table features that will not play well with this arrangement, that is the reason that the existing collapsed formatter works in its own element.

This is not a matter that is up for discussion, a number of table features and a lot of users custom styles are predicated on the row being display type block and this is not something that can be changed. I understand that this will mean a bit more work is needed to build out the display, but if it dosnt happen then this PR will not be accepted.

Looking at your code it would not take much effort to move it to a system were it gets the cell element and appends it to a container/table and then sticks it back in the row before it is made visible. Im happy to discuss approaches for this that will work best with the rest of the table.

My reason for suggesting specifically that you switch to using a table to lay things out is that it will make it much easier for users to switch between this and the existing collapse and maintain any custom styling they may have in place, improving overall table usability. But i am open to hearing other approaches you may have for the layout of the contents, but it must be in its own container and not affect the display type of the row.

Option Naming

It would be better if the name of the collapse option explained what it did differently from the main collapse option. I would propose a name of collapseEditable would be more self explanatory than collapseFlex

Editing Cell Labels

When editing the cells, the labels would need to remain in place rather than being hidden. 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.

Editable Cell Styling

The cells that are editable should not be styled differently, 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

Cheers

Oli :)

@olifolkerd olifolkerd added the PR Needs Work The PR is a good idea, but needs some updates before it can be merged label Jul 28, 2022
@lekoala
Copy link
Contributor Author

lekoala commented Jul 29, 2022

Yes all that makes sense. I don't have the time right now to investigate how to do that but if I manage to find some spare time I'll give it a try :)

initializeRow(row){
var el;

if(row.type !== "calc"){
el = document.createElement("div");
el.classList.add("tabulator-responsive-collapse");
if(this.table.options.responsiveLayout === "collapse"){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this supposed to work? el in L.160 will fail as it will be undefinied.

@@ -244,6 +289,9 @@ class ResponsiveLayout extends Module{

if(row.modules.responsiveLayout){
el = row.modules.responsiveLayout.element;
if(!el){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this problem results from L.150 by passing an undefined el.

@lekoala
Copy link
Contributor Author

lekoala commented Aug 29, 2023

@ortwic i do not intend to work on this any time soon so feel free to make your own proposal based on oli's feedback

@lekoala
Copy link
Contributor Author

lekoala commented Sep 1, 2023

ortwic#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR Needs Work The PR is a good idea, but needs some updates before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants