-
Notifications
You must be signed in to change notification settings - Fork 424
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
fixed issue #546 - broken multi column sort with invisible columns #549
fixed issue #546 - broken multi column sort with invisible columns #549
Conversation
merged 6pac/master into fork/master
fetching latest master
pulling newest origin:main
- in some cases we might want to use "disabled" but in other cases we might want to use "hidden"
@ghiscoding , @6pac this is a basic solution based on our discussion on #546! I have a fundamental issue with this solution though! if a user updates |
or well, another solution is to add a |
@@ -2765,6 +2784,31 @@ if (typeof Slick === "undefined") { | |||
} | |||
} | |||
|
|||
function setAllColumns(columnDefinitions){ | |||
var _columns = columns.slice(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you just want to make a copy, you can omit the 0 and just go with .slice()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, will change - waiting for our conversation to reach an agreement
@@ -0,0 +1,14 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please remove this file from the project, I think it's unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn! don't understand how this file always gets pushed! it doesn't exist in my code! :/ sorry! will be removed
@@ -2726,6 +2728,23 @@ if (typeof Slick === "undefined") { | |||
return columns; | |||
} | |||
|
|||
function getColumnById(columnId, useOnlyVisibleColumns){ | |||
var columnList = useOnlyVisibleColumns?getColumns():getAllColumns(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you space this code, it would be a bit more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
I would say that in any case we want to avoid pushing breaking changes as much as possible because there are thousands of people using the lib and few hundreds using my libs but I can update mine quickly. However what could potentially be done is to make the |
I've been staying out of this one, mainly because I use my own customised version of the grid, and a different customised DataView that doesn't have a lot of the limitations of the public one. Not sure if I even have this issue. I'd like to merge but the two have diverged so far that it would be a lot of work. So I'd make two comments:
|
I was thinking to leave the solution to skip the not found columns in place and instead of adding the
if someone doesn't use the new feature, the old logic would still work by changing the complete column set; but the new way would let them have a list of all columns in the background (and get it using we could then extend the
that way, the current functionality will stay unchanged, as the |
I'd really like to see your version! :D tbh, we are considering changing from |
Unfortunately, those are not things I changed. If anything, the UI capabilities are much less than the current grid. I don't have the split grid or the details view at all in my version (it forked about four years ago, before all that).
The idea of FlexTable/FlexView was that it would offer all the sorting/filtering/offline capabilities as a single object that could be used by any client side js component rather than each one 'rolling their own' filtering/sorting, and that it could be used as a lightweight wrapper for a plain js object, just progressively instantiating needed/used features. Ideally it could be used as a wrapper for a whole array of js 'class' business objects/models to add offline sync, filtering or aggregation. |
Hey folks, is this still relevant or should I close it? |
I have actually implemented the solution as described here and it works in my version. but because of this, my deployment branch is not the active main branch anymore. honestly, I have not checked the newest version to see the issue has been already resolved or not. I suppose, if none of us has worked on it, this will still be an open issue? |
closing in favor of #723 which is up to date with latest master branch |
added new property
allColumns
and 3 new functionssetAllColumns(columnDefinitions)
getAllColumns(defaultToColumns)
and getColumnById(columnId, useVisibleColumns)- the last function is used in
setupColumnSortin order to get correct column: if
allColumnsis set then use
allColumnselse use
columns` as reference list