-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Tabledata store reorganization #654
Conversation
…editable_tables
…editable_tables
…editable_tables
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.
Things seem to work on my machine, and the tests are passing.
Optional suggestion:
While I think the new organization makes sense on a file level, I find it a little confusing at the directory level. The table-data
directory contains files pertaining to not just the data for a table (i.e., the records), but also the metadata, and also some display concerns. I suggest either renaming that directory, or even further subdividing into three directories roughly along the lines:
- actual data storage
- metadata storage
- display variables storage.
However, I defer to your experience here, @pavish , since I have little knowledge in the idiomatic use of the tools in question.
@mathemancer I agree that it would make more sense to come up with a better name for the directory. Splitting it up into smaller directories would not be ideal at this point, since there is only one file per type of data. I will handle this in the upcoming PR: #671 |
@kgodey @zackkrida @dhruvkb @mr-gabe49 Please feel free to add review comments. I'll go ahead and merge this PR since it's blocking other PRs, and handle the comments in a separate PR. |
|
||
function recalculateColumnPositions(columnPositionMap: ColumnPositionMap, columns: TableColumn[]) { | ||
let left = ROW_CONTROL_COLUMN_WIDTH; | ||
const newColumnPositionMap: ColumnPositionMap = new Map(columnPositionMap); |
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.
Hey @pavish, fabulous work here! Quick question: What's the reasoning for passing columnPositionMap
inside of new Map()
? It prevents columnPositionMap
from updating correctly in line 76-78: this.columnPositionMap.update( (map) => recalculateColumnPositions(map, columnData.data), );
when calling columns.fetch()
after a column is deleted.
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.
@mr-gabe49 When a column has been previously resized, the columnPositionMap would contain a different value for width instead of the default column width. We create a new map using the existing map to retain it.
Good catch, I hadn't considered the effect of deletion.
In your PR for column deletion, we could update lines 30-32 to:
const newColumnPositionMap: ColumnPositionMap = new Map();
columns.forEach((column) => {
const columnWidth = columnPositionMap.get(column.name)?.width;
Since we have access to the old map, we could use it to get the previous width. This would handle position recalc for both resize and delete.
Related to #651
Technical details
This PR:
Breaking changes:
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin