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

ColReorder support for Row Expansion in TurboTable #4999

Closed
loicsalou opened this issue Jan 27, 2018 · 10 comments
Closed

ColReorder support for Row Expansion in TurboTable #4999

loicsalou opened this issue Jan 27, 2018 · 10 comments
Assignees
Labels
Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add
Milestone

Comments

@loicsalou
Copy link

There is no guarantee in receiving an immediate response in GitHub Issue Tracker, If you'd like to secure our response, you may consider PrimeNG PRO Support where support is provided within 4 business hours

I'm submitting a ... (check one with "x")

[x] bug report => Search github for a similar issue or PR before submitting
[ ] feature request => Please check if request is not on the roadmap already https://github.com/primefaces/primeng/wiki/Roadmap
[ ] support request => Please do not submit support request here, instead see http://forum.primefaces.org/viewforum.php?f=35

Plunkr Case (Bug Reports)
Please check my own primeng fork at: https://github.com/loicsalou/primeng.git branch "bugfix/tablerowexpansion-colreorder-issue" and launch showcase then check turbo table's expansion row demo to reproduce the issue.

Current behavior
when drag'n'dropping a column the resulting table is not correct: column is not at the correct place. Moving the last column, moving color before brand, produces an error (ERROR TypeError: Cannot read property 'header' of undefined).

Expected behavior
no error expected + dragged column should be placed at the drop position

Minimal reproduction of the problem with instructions
drag a column to a new position and check final order of columns, it's wrong

Problem analysis: easy to understand actually. The method "Table.onColumnDrop(event, dropColumn)" reorders the declared columns (for instance Vin, Year, Brand, Color) but the domHandler provides dragIndex and dropIndex from the dom and as I have setup a column for the toggle of expansion row I have 5 columns, not 4. So the columns involved in the reorder are not correct and if I move the last column (Color) I get the index 5 which does not exist in the declared columns).

I'm trying to find a solution which I can propose.

What is the motivation / use case for changing the behavior?
reorder is an important feature for my users

Please tell us about your environment:
MacOs or windows 7, Chrome (latest, Angular 5.2, prime 5.2.0-rc.3, node 8.4, npm 5.6.0)

  • Angular version: 5.2.0
    the issue is definitely coming from Primeng

  • PrimeNG version: 5.2.0-rc.3

  • Browser: Chrome latest, Safari

  • Language: [all | TypeScript X.X | ES6/7 | ES5]
    Typescript

  • Node (for AoT issues): node --version =

@loicsalou
Copy link
Author

So I have a solution for the issue in my context. Not really sure it is acceptable.
Actually the problem seems conceptual. when reordering columns, PrimeNg detects dragged column then dropped column correctly. In the end the API uses dragged and dropped column's parent (the TR) and loops thru children, searching for TDs.
In the case of expansion row we need an extra first column to add the toggler icon which opens/closes the row. problem is that it sees the toggler as a regular columns but it's not, it's an extra column.
So my solution searches thru the parent's children for columns having preorderablecolumn attribute.

Here comes the conceptual IMHO. Nothing forces developers to set the pReorderableColumn directive on each column of the table. in this case my solution will not work either.

Any way I will propose this as a PR in case you think it's acceptable solution.
Probably you can find a better approach, I just hope this can help in some way.

@cagataycivici
Copy link
Member

Please create a plunkr using;

http://plnkr.co/edit/Nxca3ovCbtvx9kpGds0q?p=preview

So that we can see the issue in action

@cagataycivici cagataycivici added the Status: Discussion Issue or pull request needs to be discussed by Core Team label Jan 31, 2018
@loicsalou
Copy link
Author

I'm new to plunkr so I hope this will be ok: http://plnkr.co/edit/ZnKbvMJFsS94EYcKG6Wo?p=preview

as you can see, drag and dropping is not working correctly especially if you drag and drop rightmost column, you get an error.

my PR #5000 proposes a fix, I hope this helps.

@marslan2037
Copy link

marslan2037 commented Feb 6, 2018

you have this in your code before header loop start,
< th style="width: 2.25em" > < / th > , just remove it from here and add it inside loop then you will have no problem, this is not a exact solution but for now i think this is the solution we have

@marslan2037
Copy link

exact issue is custom column before loop

@loicsalou
Copy link
Author

This column has a technical goal (expand / collapse the row) and does not belong to the model of the data displayed in the table. Of course that would do the trick but IMO this is more some sort of hack than a solution. Problem comes from the DomHandler which should check wether a column belongs to the data or not.

@kareljan
Copy link

having the same problem

@marslan2037
Copy link

@loicsalou you are right

@loicsalou
Copy link
Author

@cagataycivici any chance that this issue be fixed in a short coming fix release ? #5000 provides a fix. Thanks

@cagataycivici cagataycivici self-assigned this Mar 2, 2018
@cagataycivici cagataycivici added Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add and removed Status: Discussion Issue or pull request needs to be discussed by Core Team labels Mar 2, 2018
@cagataycivici cagataycivici added this to the 5.2.1 milestone Mar 2, 2018
@cagataycivici cagataycivici changed the title turbo-table: colReorder issue with expansion row ColReorder support for Row Expansion in TurboTable Mar 2, 2018
@cagataycivici
Copy link
Member

Added to plan of 5.2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants