Hacks are explained in code comments.
Depends on D6642
Details
Details
- Tested both iOS and Android
- Made sure there was no flicker
- Tested @-tag in both middle and end of text
- Tested moving cursor after selecting @-tag
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/components/selectable-text-input.react.ios.js | ||
|---|---|---|
| 3 | In most of the places we're importing fp versions of lodash functions. Should we keep it consistent? For me, non-fp versions are usually a lot more readable, but there might be a reason for our current preference. | |
| 110–113 | This seems risky to me. I can imagine that it might work differently on different OS or RN versions and we would need to test it really carefully in the future when updating. I'm also wondering if quickly selecting two users one after another might break it, but it is only a theoretical issue because no one can be quick enough and that will also require rendering a new typeahead, so we don't need to worry about it. But overall, I think it might be a good idea to protect us against a correct behavior where additional events aren't fired. So instead of ignoring the events when the counter is positive, save the event somewhere and fire it if extraneous events won't appear (after some timeout). The change would be to always assign to pendingSelectionEventRef.current and always call the debounced function - if the additional events appear, it would behave as this solution, but if these events won't show, the last event would be send. | |
| native/components/selectable-text-input.react.ios.js | ||
|---|---|---|
| 3 | Good call, should be consistent to avoid bloating bundle size unnecessarily, especially since debounce is the same between FP and non-FP versions. I'm open to getting rid of the FP stuff but it's somewhat useful with the _flow command | |
| 110–113 | We can't do this since we don't have any guarantee that another event will fire. If the user simply selects a @-mention and does nothing else, all that will happen is that one or two spurious events will fire. No event fires for our controlled selection update (which is probably what we expect – it would be weird if our controlled changes fire events back to us) I think it would be bad to allow these spurious events through in the situation where no further events fire, since that is an expected case
I agree that it's risky. That said, the negative effects of ignoring selection events are not so bad... it leads to a weird effect for the user where their selection update is ignored one or two times, but then it works fine after that. I think the only alternative is the one we were previously using, where instead of ignoring these bad events we let them through, but then overwrite the selection later. This leads to a bad visual experience and I'd prefer to avoid it. | |
| native/components/selectable-text-input.react.ios.js | ||
|---|---|---|
| 3 | Actually this is the same path we use elsewhere in the app: # git grep debounce | grep -v flow-type | grep -v yarn.lock | grep import keyserver/src/socket/socket.js:import _debounce from 'lodash/debounce'; native/components/selectable-text-input.react.ios.js:import _debounce from 'lodash/debounce'; web/utils/tooltip-utils.js:import _debounce from 'lodash/debounce'; Let's stick with that to avoid bloating bundle size. | |