new(DataTable): Add xLarge row size. Fix forceUpdate logic. #367
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
williaster
requested review from
alecklandgraf,
hayes,
milesj and
stefhatcher
as code owners
Collaborator
Size Changes
View raw build statsPrevious (master){
"apollo": {
"esm": 10832,
"lib": 14147
},
"app-shell": {
"esm": 12906,
"lib": 19874
},
"composer": {
"esm": 68247,
"lib": 101805
},
"core": {
"esm": 576067,
"lib": 721289
},
"forms": {
"esm": 36180,
"lib": 48408
},
"icons": {
"esm": 156355,
"lib": 205626
},
"layouts": {
"esm": 15298,
"lib": 20770
},
"metrics": {
"esm": 5467,
"lib": 7729
},
"test-utils": {
"esm": 4279,
"lib": 5937
}
}Current{
"apollo": {
"esm": 10832,
"lib": 14147
},
"app-shell": {
"esm": 12906,
"lib": 19874
},
"composer": {
"esm": 68247,
"lib": 101805
},
"core": {
"esm": 576205,
"lib": 721427
},
"forms": {
"esm": 36180,
"lib": 48408
},
"icons": {
"esm": 156355,
"lib": 205626
},
"layouts": {
"esm": 15298,
"lib": 20770
},
"metrics": {
"esm": 5467,
"lib": 7729
},
"test-utils": {
"esm": 4279,
"lib": 5937
}
} |
williaster
commented
Apr 30, 2020
| const { sortBy, sortDirection } = this.state; | ||
| const dimensionsChanged = width !== prevProps.width || height !== prevProps.height; | ||
| const sortChanged = | ||
| sortBy !== prevState.sortBy || |
Contributor
Author
There was a problem hiding this comment.
we already invoke this.cache.clearAll(); when updating sortBy or sortByDirection, so this was doing a double update
williaster
force-pushed
the
chris--datatable-timeout-n-xlarge
branch
from
5c74d88 to
1376e36
Compare
stefhatcher
reviewed
May 1, 2020
|
|
||
| type HeightMap = { | ||
| [key: string]: number; | ||
| [key in RowHeightOptions]: number; |
stefhatcher
approved these changes
May 1, 2020
stefhatcher
changed the title
milesj
reviewed
May 1, 2020
| if (dynamicRowHeight) { | ||
| // We need to make sure the cache is cleared before React tries to re-render. | ||
| window.setTimeout(() => { | ||
| if (this.timeoutId) window.clearTimeout(this.timeoutId); |
Contributor
There was a problem hiding this comment.
If a timer is already set, shouldn't that one just run instead of clearing and creating a new one?
milesj
reviewed
May 1, 2020
| export type DataTableRef = (instance: DataTable) => void; | ||
| export type TableRef = React.RefObject<Table>; | ||
| export type RowHeightOptions = string; | ||
| export type RowHeightOptions = 'micro' | 'small' | 'regular' | 'large' | 'xLarge' | 'jumbo'; |
Contributor
There was a problem hiding this comment.
Lets just call it xlarge for consistency.
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
to: @milesj @stefhatcher @hayes @alecklandgraf
cc @schillerk
Description
This PR
DataTable:xLargerowHeighttype fromstringto a union of possiblestringforceUpdatecalls incomponentDidUpdateMotivation and Context
I've been trying to upgrade to lunar v3 and some of our tables are pretty broken, especially for dynamic row height. Adding an additional fixed row height will alleviate some of these issues for us, and I'm hoping the
forceUpdatelogic may help. I'm not sure if there have been regressions, but it's pretty easy to generate a stack trace in the storybook now. Go here, expand the row, then sort a column 😱I couldn't think of a great way to remove the force updates, but I tried to clean it up a bit. You can still get the stack trace from resizing the window many times, unfortunately the
AutoSizerdoesn't support adebounceproperty 😢Testing
xLargerow height renders as expectedScreenshots
xLarge (no story changes)
Checklist