-
Notifications
You must be signed in to change notification settings - Fork 0
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
187887881 selected row sync #24
Conversation
- very hacky and not performant at the moment - needs revision - might need to adopt a "get notification -> ask codap" pattern - which means removing "get notification -> get data from notification" pattern
- use the ref values in handleCaseSelectionInCodap, rather than passing in the state vars, allowing us to use the ref values in handleCaseSelectionInCodap,allowing us to remove latitude, longitude, and dayOfYear from handleDataContextChange dependencies
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.
Looks good 👍
I left a couple of suggestions in the inline comments. If they don't cause any issues, I'd suggest applying them. If they do cause issues, let's go ahead and merge this PR as it is.
return null; | ||
} | ||
}).then((selectionResult: any) => { | ||
if (!selectionResult.success) { |
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.
Wouldn't that line fail with null
passed as selectionResult when the block above returns null
?
debouncedUpdateRowSelectionInCodap( | ||
currentDayLocationRef.current._latitude, | ||
currentDayLocationRef.current._longitude, | ||
day |
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.
Since this is not part of any useCallack
and/or useEffect
, you can freely use state here and simply call:
debouncedUpdateRowSelectionInCodap(latitude, longitude, day)
const getUniqueLocationsRef = useRef(getUniqueLocationsInCodapData); | ||
|
||
const handleDayUpdateInTheSimTab = (day: number) => { | ||
currentDayLocationRef.current._dayOfYear = day; |
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 think this line can be anywhere in the functional component body, e.g. right below state declarations. As any state update will trigger re-render, and this line will be executed during this re-render.
const newDayChoice = `${_dayOfYear}` !== `${selectedDay}`; | ||
if (rowInLocation && newDayChoice) { | ||
setDayOfYear(selectedDay); | ||
currentDayLocationRef.current._dayOfYear = selectedDay; |
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.
As above, probably you don't need this ref update here.
}; | ||
|
||
initialize(); | ||
}, []); | ||
|
||
const handleDataContextChange = useCallback(async (listenerRes: ClientNotification) => { | ||
console.log("| dataContextChangeNotice: ", listenerRes); |
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.
console.log to remove?
_longitude: longitude, | ||
_dayOfYear: dayOfYear | ||
}; | ||
}, [latitude, longitude, dayOfYear]); |
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 think you don't need to wrap that in useEffect. It can be anywhere in the functional component body. I usually place it right after the ref declaration.
@@ -23,6 +23,7 @@ export const useCodapData = () => { | |||
if (result.success) { | |||
let dc = result.values; | |||
let lastCollection = dc.collections[dc.collections.length - 1]; | |||
console.trace(); |
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.
debug line to remove?
PT-187887881 Selected Row Sync
PT-188109247 When Sim is accessed, it moves to the front
.caseSearch