issue 1478/column width #1505
issue 1478/column width #1505
Conversation
kaulfield23
requested review from
richardolsson,
ziggabyte,
rebecarubio and
niklasva82
There was a problem hiding this comment.
This PR should not have been merged as is. It is much too complicated to achieve what it does. It could have used useConfigurableDataGridColumns correctly and just worked, instead of making changes to the interface of the useConfigurableDataGridColumns.
There were red flags here (see below) and I think it was a mistake to approve it. I have asked @kaulfield23 to submit a new PR that replaces the changes in this PR with more reasonable ones. All in all, a good learning experience for anyone who wants to learn from it.
| @@ -98,7 +98,7 @@ export default function useConfigurableDataGridColumns( | |||
| }; | |||
| } | |||
|
|
|||
| function loadConfig(storage: StorageBackend, key: string): ColumnConfig { | |||
| export function loadConfig(storage: StorageBackend, key: string): ColumnConfig { | |||
There was a problem hiding this comment.
This is a major red flag. Never export something like this without first fully understanding it (and never approve such exports without understanding whether they're necessary).
There are two red flags here:
- In general, exporting something that was previously not exported is a red flag in itself. Assume that the person who wrote the code refrained from exporting for a reason. Make sure you understand why, before going against that original design.
- The name of this module is
useConfigurableDataGridColumns. It had a single default export by the same name (and some relevant types). This is a common pattern. The name of the module indicates that it should have a single export. Adding another export probably goes against the original author's design, and if it does, there better be a good reason (which requires first understanding that design).
There was a problem hiding this comment.
Thanks for the feedback! It is really helpful and I will make a note of it so it wouldn't be repeated!
| ...columnTypes[col.type].getColDef(col, accessLevel), | ||
| })), | ||
| ]; | ||
| const { setColumnWidth } = useConfigurableDataGridColumns( |
There was a problem hiding this comment.
This hook returns an object which includes the re-configured columns. It would have been possible to just do:
const { columns: gridColumns, setcolumnWidth } = useConfigurableDataGridColumns((And make the necessary changes to avoid the naming conflict of gridColumns).
This would have made all other changes in this PR unnecessary.
Description
This PR fixes a bug that columns' widths in view is not saved as user set. It goes back to the default width(150) after sorting or refreshing the page.
Screenshots
New.View.Mozilla.Firefox.2023-09-06.11-44-44.mp4
Changes
exporttoloadConfigonColumnResizeNotes to reviewer
I used
loadConfig. I found another bug(#1039 ) while I work on this, and thoughtloadConfigcould be useful for fixing that issue later onRelated issues
Resolves #1478