-
Notifications
You must be signed in to change notification settings - Fork 99
[WIP] fix(DataGrid): Allow columns with empty titles to be used in DataGrid
#1948
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
base: main
Are you sure you want to change the base?
[WIP] fix(DataGrid): Allow columns with empty titles to be used in DataGrid
#1948
Conversation
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 the tanstack id
is only browser specific, then this is great.
If the id is used for updates to the server (ex column sorting input values), then we should raise an error server-side before it is even sent to the browser as the column names will not match.
Narwhals has a restriction of non-duplicated column names. It's fair if we add non-empty column names for tanstack.
* @param initName String to start the search with. | ||
* @returns A string that does not exist in columns and thus can be used as column ID. | ||
*/ | ||
function findUnusedColumnName(columns: ReadonlyArray<string>, initName = " "): string { |
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 we move initName
into the function as this will only hold true if the initName
is " "
.
If other column names can be expanded, then they could both resolve to the same " "
string.
However, if we know that ""
is the only one being expanded and there are no duplicate names, then this function is only called once... which is safe.
Changing to something more specific...
function findUnusedColumnName(columns: ReadonlyArray<string>, initName = " "): string { | |
function emptyColumnNameToId(columns: ReadonlyArray<string>): string { | |
let initName = " "; |
tests/playwright/shiny/bugs/1844-datagrid-empty-column/test_1844_datagrid_empty_column.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Barret Schloerke <[email protected]>
DataGrid
DataGrid
Co-authored-by: Barret Schloerke <[email protected]>
Co-authored-by: Barret Schloerke <[email protected]>
Fixes: #1844
The issue stems from the fact that TanStack uses column headers to create unique IDs, and an empty string is not a valid ID. Thus, my solution is to create a unique ID for the column with an empty header. Truth be told, there's no particular reason not to assign IDs equal to their corresponding headers for all the other columns, other that trying to change as little as possible.
Finding a unique ID is done through
findUnusedColumnName()
which was somewhat inspired by how {tibble} solves column name issues. In this case, the algorithm appends a space until it arrives at a string that doesn't exist in the list of column names. Again, the choice of space is arbitrary. What is important though is to keep in mind that this approach will fail if there is more than one column with an empty name. However,DataGrid
currently doesn't work with duplicated names anyway, and generating unique IDs for all columns would probably be the solution.Side note, I suspect that these large diffs in JS files are due to me using Windows, though I made sure that the output doesn't replace any LF with CRLF.