Skip to content
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

add selectedMOIs to saved search #4570

Merged
merged 5 commits into from
Jan 14, 2025
Merged

add selectedMOIs to saved search #4570

merged 5 commits into from
Jan 14, 2025

Conversation

jklugherz
Copy link
Contributor

@jklugherz jklugherz commented Jan 6, 2025

it works!

selectedmoi.mov

@hanars
Copy link
Collaborator

hanars commented Jan 6, 2025

I can pair on this with you if you want, but at a high level I think changing the behavior for onChange the way you did is probably not the right direction to get it to populate properly. For the issue with initial form population you mention, I think you need to change the componentDidUpdate callback on the LocusListSelector thats called when the loading of the locus list completes, and/or change formatPanelAppItems to handle the MOI filtering . And for the actual onChange behavior itself I think you just want to move the MOI selector up a level so its not nested under panelapp anymore

@jklugherz
Copy link
Contributor Author

@hanars I implemented the PaLocusListSelector using the shouldComponentUpdate and componentDidUpdate approach (and removed the new listener component) but I'm still seeing the values reset, after the page loads.

Screen.Recording.2025-01-07.at.4.49.05.PM.mov

I logged the value inside of the PaMoiSelector render() and the first time it's logged it is accurately set (as [XD]) but on subsequent renders it's replaced with undefined. Do you have any ideas as to what's happening here?

@hanars
Copy link
Collaborator

hanars commented Jan 8, 2025

Try looking into the componentDidUpdate method of LocusdListSelector which is what triggers the on change event when loading a locus list succeeds and see what it is using to trigger the onChange in that helper - its possible that its not properly passing throught the MOIs there

@jklugherz jklugherz marked this pull request as ready for review January 13, 2025 18:55
@jklugherz jklugherz requested a review from hanars January 13, 2025 18:55
@jklugherz
Copy link
Contributor Author

@hanars I got this to work!! had to really understand what was happening during the lifecycle methods to see when and where the locus state was being updated.

Copy link
Collaborator

@hanars hanars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay so excited this is working! One intr suggestion on syntax but otherwise looks awesome!

Comment on lines 90 to 98
const filteredItems = items.filter((item) => {
if (!selectedMOIs || selectedMOIs.length === 0) {
return true
}
const initials = moiToMoiInitials(item.pagene?.modeOfInheritance, false)
return selectedMOIs.some((moi) => initials.includes(moi))
})

return filteredItems.reduce((acc, item) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be slightly neater as a single reduce with a helper method

Suggested change
const filteredItems = items.filter((item) => {
if (!selectedMOIs || selectedMOIs.length === 0) {
return true
}
const initials = moiToMoiInitials(item.pagene?.modeOfInheritance, false)
return selectedMOIs.some((moi) => initials.includes(moi))
})
return filteredItems.reduce((acc, item) => {
const hasSelectedMoi = (item, selectedMOIs) => {
if (!selectedMOIs || selectedMOIs.length === 0) {
return true
}
const initials = moiToMoiInitials(item.pagene?.modeOfInheritance, false)
return selectedMOIs.some((moi) => initials.includes(moi))
}
...
return items.reduce((acc, item) => {
if (!hasSelectedMoi(item, selectedMOIs) {
return acc
}
...

@jklugherz jklugherz merged commit 01899fa into dev Jan 14, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants