[native] Check isInvalidPinSource on native
ClosedPublic
Actions

Authored by rohan on Oct 30 2023, 9:26 AM.

Details

Reviewers

atul
ginsu
ashoat

Commits

rCOMM594eeb632361: [native] Check isInvalidPinSource on native

Summary

There was an inconsistency where I check isInvalidPinSource on web, but not on native. This diff corrects that. The following diff will cover unifying this logic into a shared helper so the checks are done in one place.

Resolves https://linear.app/comm/issue/ENG-5618/check-isinvalidpinsource-on-native-in-the-toolitp

Test Plan

Confirmed that message pinning on native still works when expected:

For text messages
For multimedia messages
In sidebars if the message is not the first message

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Oct 30 2023, 9:26 AM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptOct 30 2023, 9:26 AM

Harbormaster completed remote builds in B23573: Diff 32503.Oct 30 2023, 9:43 AM

rohan requested review of this revision.Oct 30 2023, 9:43 AM

rohan added a child revision: D9638: [lib/web/native] Unify logic to check canTogglePins in web and native tooltips.Oct 30 2023, 12:18 PM

How come this adds isInvalidPinSource for both web and native? Is that because the diff that adds it to one and not the other hasn't been landed yet? Does that diff now need to be abandoned?

This revision is now accepted and ready to land.Oct 30 2023, 12:22 PM

In D9634#282161, @ashoat wrote:

How come this adds isInvalidPinSource for both web and native? Is that because the diff that adds it to one and not the other hasn't been landed yet? Does that diff now need to be abandoned?

Sorry, would you mind clarifying what you mean by "how come this adds isInvalidPinSource for both web and native"?

This diff only adds it into native to be consistent with when I added it for the web tooltip in D9445. In our meeting today we noticed that I forgot to include it on the native side.

The next diff in this stack is the 2nd step of what we discussed (unifying the logic / extracting common checks / etc)

Ah sorry, I thought this was updating web and native, but it's actually updating two places in native. We should obviously factor this out so we're not copy-pasting code, but I suppose that will be handled in the subsequent diff (as indicated in the diff description)