Comments virtualized list bugs #146
Conversation
| useEffect(() => { | ||
| // What does this do? | ||
| if (store.router.params.currentItem === 'comments_list' && store.router.params.currentItemId) { | ||
| const filteredData = commentsData?.presentationItems.find( | ||
| const currentPresentationItem = presentationItemsData?.presentationItems.find( | ||
| (item) => item.id === store.router.params.currentItemId | ||
| ); | ||
| store.campaign?.setCommentsList({ | ||
| commandGroupIds: Array.from(filteredData?.commandGroupIds || []), | ||
| commandGroupIds: Array.from(currentPresentationItem?.commandGroupIds || []), | ||
| }); | ||
| } | ||
| }, [store.campaign.commentsList.commandGroupIds, store.router.params.currentItem, store.router.params.currentItemId]); | ||
| }, [presentationItemsData, store.router.params.currentItem, store.router.params.currentItemId]); |
There was a problem hiding this comment.
@sharplessHQ, can you sort out what this useEffect is for? and also the required presentationItemsData? Could this be removed or is it important? Could we write a comment to explain the purpose?
There was a problem hiding this comment.
This is needed to render the page correctly under a specific comment type. Try refresh the page or sorting without this useEffect.
There was a problem hiding this comment.
- Sorting under a specific type won't work with this change.'
(As we talked yesterday, this dependency change would cause an issue and i used the circular way TEMPORARILY since i couldn't find another way to make it work at that moment. But i forgot what the issue was... Now i remember when checking your PR, it's the sorting under a specific type.)
There was a problem hiding this comment.
Got it, Thanks. I will try to resolve it.
There was a problem hiding this comment.
I combined the 4 useQuery into 2 that provide different calls based on the requested state. I also removed the global store.campaigns.commentList because it is local to the Comments.tsx component.
BUT, this definitely needs review. @sharplessHQ and @GoldingAustin please look at the final state of Comments.tsx and see if it makes sense to combine these useQueries in this way.
I've tested and everything seems to work fine. All ways returns the correct list, correctly sorted, and on initial load.
There was a problem hiding this comment.
It looks good and works right to me. I was planning to combine after i finished up the second set of queries, but then that refresh/sorting thing i mentioned above made me concerned combining might cause more issues. I guess my 2 separate sets of queries are the thing causing this issue...
Thanks for fixing this! :-)
My only comment is about the ProgressBar bug^
|
Using the gt.redeye file, I clicked into Explorer Mode, then the Comments tab. If you click on Favorited Comments, which has 0, the page looks like it's continuously loading but never does. We saw this in a previous PR and it was resolved, so whatever fixed that behavior must have been affected. No console errors. |
| {commandGroupIdData?.commandGroupIds?.length === 0 ? ( | ||
| isLoading ? ( | ||
| <ProgressBar intent={Intent.PRIMARY} /> | ||
| ) : ( | ||
| store.campaign.commentsList.commandGroupIds.map((commandGroupId) => ( | ||
| <CommentGroup | ||
| cy-test="comment-group" | ||
| key={commandGroupId} | ||
| commandGroupId={commandGroupId} | ||
| toggleNewComment={state.toggleNewComment} | ||
| newComment={state.newComment} | ||
| expandedCommandIDs={state.expandedCommandIDs} | ||
| removeExpandedCommandID={state.removeExpandedCommandID} | ||
| /> | ||
| )) | ||
| <MessageRow>No comments</MessageRow> | ||
| ) | ||
| ) : data?.commandGroupIds.length === 0 ? ( | ||
| <MessageRow>No comments</MessageRow> | ||
| ) : isLoading ? ( | ||
| <ProgressBar intent={Intent.PRIMARY} /> | ||
| ) : ( |
There was a problem hiding this comment.
Using the gt.redeye file, I clicked into Explorer Mode, then the Comments tab. If you click on Favorited Comments, which has 0, the page looks like it's continuously loading but never does. We saw this in a previous PR and it was resolved, so whatever fixed that behavior must have been affected. No console errors.
- Fix this: caused by the new render logic in the
Commentsreturn. It doesn't account for if there are 0 comments.
🗣 Description
The Comments virtualized list scrolling was unusable. It jumped all over the place while scrolling. There were 2 issues
Comments.tsxuseEffect. The useEffect body updated a property that was also set as a dependency of the useEffect. This caused constant re-rendering of the list.🧪 Testing
Some of the Comment list code was edited, and is pretty complex in terms of queries (TODO: should be simplified). This means that we need to test everywhere comments are present
Features
📷 Screenshots (if appropriate)