some fixes when setting noImplicitAny: true #5840
Conversation
| if (linkIsExcludedFromPreview(href)) | ||
| continue; | ||
| const id = linkTag.getAttribute("id"); | ||
| const rel = linkTag.getAttribute("rel") | ||
| const id = linkTag.getAttribute("id") ?? ''; |
There was a problem hiding this comment.
Should be undefined rather than '' I think (and same for others in this function)
There was a problem hiding this comment.
Thanks a bunch for this. I didn't look over everything, but aggressively skimmed half the changes. I could imaaaaaagine some of the changes from undefined to '' causing behavior changes, but it seems unlikely. All of the type changes I trust you for and/or seem correct.
Please fix the tests before merging, obvs
| const handleScroll = (e) => { | ||
| const isAtBottom = Math.abs((e.target.scrollHeight - e.target.scrollTop) - e.target.clientHeight) < 10; | ||
| const handleScroll = (e: React.UIEvent<HTMLDivElement>) => { | ||
| const isAtBottom = Math.abs((e.currentTarget.scrollHeight - e.currentTarget.scrollTop) - e.currentTarget.clientHeight) < 10; | ||
|
|
||
| // If we are not at the bottom that means the user has scrolled up, | ||
| // in which case never autoscroll to the bottom again | ||
| if (!isAtBottom) | ||
| setUserHasScrolled(true); | ||
|
|
||
| // Start loading more when we are less than 1 page from the top | ||
| if (!loadingMoreComments && commentCount < totalComments && e.target.scrollTop < e.target.clientHeight) | ||
| if (!loadingMoreComments && commentCount < totalComments && e.currentTarget.scrollTop < e.currentTarget.clientHeight) |
There was a problem hiding this comment.
FYI @Will-Howard this is one of the few actual code changes in this PR. I'm not 100% sure how to test this change; adding the React.UIEvent<HTMLDivElement> type to the event caused it to complain that target didn't exist, but currentTarget does. According to DefinitelyTyped/DefinitelyTyped#11508 (comment) it seems like we should probably be using currentTarget, not super confident.
There was a problem hiding this comment.
Ah, this whole file can actually be deleted now. We are now using the TagSubforumPage2 component which doesn't use this (instead of TagSubforumPage), and I don't think there's any chance of going back at this point
| @@ -78,7 +78,7 @@ const RecentDiscussionTag = ({ tag, refetch = () => {}, comments, expandAllThrea | |||
|
|
|||
| const lastVisitedAt = markedAsVisitedAt || tag.lastVisitedAt | |||
| const lastCommentId = comments && comments[0]?._id | |||
| const nestedComments = useOrderPreservingArray(unflattenComments(comments), (comment) => comment._id); | |||
| const nestedComments = useOrderPreservingArray(unflattenComments(comments), (comment) => comment.item._id); | |||
There was a problem hiding this comment.
@Will-Howard removing the implicit typing in useOrderPreservingArray caused this to error - I think it may have been silently failing to work, since CommentTreeNode doesn't have an _id, it's a container with an item (which is the comment that has an _id). But not sure what functional tests I ought to be trying here
There was a problem hiding this comment.
Thanks for fixing! I thought this had already been fixed but maybe it got lost in a merge or the PR was closed 🤷♂️. The functional test would be that if there are several comments being rendered in the card and you reply to the one at the bottom, it shouldn't jump to the top
|
🎉 about this finally getting merged |
┆Issue is synchronized with this Asana task by Unito