You should always be as specific as possible in a dep list. Rather than specifying modalContext here, you should specify modalContext.setModal. That way onClick will only be redefined if modalContext.setModal changes, rather than being redefined whenever modalContext changes

Instead of extracting uri specifically, please do what every out ConnectedComponent does and spread every prop. That way we won't have to update this code when a new prop is introduced

Can we make all of these $ReadOnly?

See comment above. In general, when doing something new you should look to see how it's done in the codebase. If you had looked at any ConnectedComponent, you would see we're spreading all props instead of individually pulling each of them out...

In an ideal world this isn't a comment I need to leave, it's just something you learn by reading code.

You're converting a class component into a function component.

In general, each diff should have a concrete, specific purpose. This is extra important for these huge diffs. Huge diffs should be dead simple. As soon as you sneak changes like this in, you ruin the simplicity and make it hard to review.

Please pull this refactor out of this diff. If you feel like that the class-to-function refactor is necessary to make the setModal refactor easier, that's fine – you can just pull the class-to-function refactor into its own preceding diff.

By attempting this refactor here, you are asking for trouble. Class-to-function refactor should be trivial, but you're still new to a lot of this stuff. So your class-to-function refactor diff is going to end up needing revisions, which because you've bundled it all together into this unrelated diff, are going to end up blocking this diff.

Concretely, here you are introducing a callback that gets recreated on every render. We've talked repeatedly over the past several days about React.useCallback, why it exists, its purpose, etc. Here it seems clear you still don't understand its purpose.

onClickLogIn needs to be wrapped in a React.useCallback.

Yeah that makes sense, but it should not be in the same diff. A huge diff like this can only exist if it is immaculately well-scoped. Please factor this change out into a separate (probably preceding) diff

Please avoid defining threadInfo twice.

The core problem here is that you aren't spending time figuring out the right convention for doing something.

When you're righting code like this, you should know that we're already doing something similar in the codebase. You must've encountered this already when reading code, but even if you haven't – you should know that this isn't the first time somebody is writing BaseProps/Props.

So you should start by finding an existing implementation, and trying to follow what the existing convention is.

This comment is the same high-level comment as the one about using object-spread in the ConnectedComponent. Your job isn't just to find something that works – it's to maintain the consistency of the codebase; to learn and implement best practices/conventions.

Once again, please don't do this... use object spread so the next person to add a prop to this component doesn't have to also edit this line

1. As discussed yesterday, please undo this naming change
2. Even if we agreed on this naming change, it should not be in this diff. This diff has one purpose, and it is wayyyy too huge/unwieldly for you to even consider stuffing unrelated changes in there

Please don't do this – use object spread

We agreed that this shouldn't go here! Why is it back here now?

Be maximally specific in your dep list

I changed all instances on clearModal on the Modal component that you commented on.

re-reading my diff, I think this makes sense.

Everywhere else in this diff, you delete functions named clearModal and replace them with modalContext.clearModal.

This appears to be the only place you left the clearModal function in place. What's the reason for this inconsistency?

Not sure what you mean

1. Please add a newline here
2. The exported component's onClose property is completely useful, because it always get overwritten by the useModalContext hook
   • Please don't have useless props in your components
   • Are you 100% sure that every single use of Modal's onClose prop just assigns modalContext.clearModal? Are there are any modals that do other stuff when they are closed? It seems really risky to be rolling this huge change into this unrelated diff. If you feel strongly about this change, can you factor it out into its own diff? Otherwise, please just undo the change

Let's rename this to clearModal, to match every single other usage. We want Modal's prop to be called onClose, but for every case where we are simply pulling out clearModal from ModalContext, let's be consistent about naming

Why is this prop optional?

This prop does not appear to be used

Why are we still defining this here

Sounds good. I'm going to take another look at this diff and make sure everything is consistent.

Shouldn't have been! I didn't use BaseProps when defining the ConnectedMultimedaContainer.

yeah, this whole ConnectedSidebarListModal didn't need to happen either.

Should have just used the context.

1. What's the point of declaring this component?
2. You are providing the children prop twice. There's no need to do that... <Modal {...props} /> is enough to forward the children prop