-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix row id mapping in object detail modal displaying #40874
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff 2cf9432...25cc3e8.
|
|
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.
It'd be great if we could fix #34070 as well.
let objectId; | ||
|
||
if (this.state.IDColumn) { | ||
objectId = this.props.data.rows[rowIndex][this.state.IDColumnIndex]; | ||
} else { | ||
const map = this.props.PKRowIndexMap; | ||
|
||
objectId = | ||
Object.keys(map).find(key => map[key] === rowIndex) ?? rowIndex; | ||
} | ||
|
||
return () => { | ||
this.props.onZoomRow(objectId); | ||
}; |
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.
I don't know if there's a problem, but I'd feel safer if all this.props
and this.state
references happened inside the returned callback (to avoid referencing stale props when callback is executed).
let objectId; | |
if (this.state.IDColumn) { | |
objectId = this.props.data.rows[rowIndex][this.state.IDColumnIndex]; | |
} else { | |
const map = this.props.PKRowIndexMap; | |
objectId = | |
Object.keys(map).find(key => map[key] === rowIndex) ?? rowIndex; | |
} | |
return () => { | |
this.props.onZoomRow(objectId); | |
}; | |
return () => { | |
let objectId; | |
if (this.state.IDColumn) { | |
objectId = this.props.data.rows[rowIndex][this.state.IDColumnIndex]; | |
} else { | |
const map = this.props.PKRowIndexMap; | |
objectId = | |
Object.keys(map).find(key => map[key] === rowIndex) ?? rowIndex; | |
} | |
this.props.onZoomRow(objectId); | |
}; |
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.
well, this function is re-calculated every time you hover over a row
hoverDetailEl.onclick = this.pkClick(rowIndex)
or press "Enter" - this.pkClick(hoveredRowIndex)(event);
I'd not worry about stale props
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.
changed it in fa0289f
@@ -77,6 +80,7 @@ function pickRowsToMeasure(rows, columnIndex, count = 10) { | |||
|
|||
const mapStateToProps = state => ({ | |||
queryBuilderMode: getQueryBuilderMode(state), | |||
PKRowIndexMap: getPKRowIndexMap(state), |
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.
There's a very similar bug: #34070
Unfortunately this PR does not seem fix it.
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.
seems it's a different problem, which requires more investigation
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.
fa0289f should fix it
// and we'll not know which object to show | ||
export const getRowIndexToPKMap = createSelector( | ||
[getFirstQueryResult, getPKColumnIndex], | ||
(result, PKColumnIndex) => { |
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.
nit: lowercase pkColumnIndex
?
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.
actually it's an abbreviation and it's used in other places, so I'd not change it (I came up to the same naming in the component in the initial version of the PR)
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.
* Map row id on object detail id * Add a fallback * Add e2e test * Add a limit * Provide a fix for #34070 * Simplify test
* Map row id on object detail id * Add a fallback * Add e2e test * Add a limit * Provide a fix for #34070 * Simplify test
Closes #39477
Closes #34070
Description
we have a not straightforward logic on detecting what the data should be shown after clicking on a table row to show object detail modal. This PR reuses logic of mapping row id to the entity id
How to verify
Demo
Before
Screen.Recording.2024-04-02.at.20.32.08.mov
After
Screen.Recording.2024-04-02.at.20.31.16.mov
Checklist