This is component that takes currently typed text, regex matches, suggested users and creates overlay with clickable buttons that insert usernames into text input.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Cleaned up and added PanGestureHandler so the component still takes pan/scroll events even ig it's not scrollable
| native/chat/typeahead-tooltip.react.js | ||
|---|---|---|
| 75–80 ↗ | (On Diff #21347) | This isn't a good explanation... what "scroll/pan events" needed to be handled? Please try revising this. You might have to read and rewrite it eg. 3-4 times before it's ready. Once it's ready, can you make sure somebody other than me reviews it before I take a look? |
| native/chat/typeahead-tooltip.react.js | ||
|---|---|---|
| 75–80 ↗ | (On Diff #21347) | You should explain first the bug that you were seeing, and then explain why this hack avoids the issue. You should also be honest and direct that this is a hack, and that we're not actually using it for anything – just to get around the issue |
Comment Actions
Updated comment. I feel like now it's really exhaustive explantion of both the bug and fix.
Or at least as exhaustive as comment can get.
Comment Actions
Once it's ready, can you make sure somebody other than me reviews it before I take a look?
Did you do this? I wonder if somebody else could've caught the typos. I'd love to get to a world where I'm not the only person on the team who can review English like this...
| native/chat/typeahead-tooltip.react.js | ||
|---|---|---|
| 75–91 ↗ | (On Diff #21366) | Paragraphs are hard to discern here, maybe try adding some empty lines to separate them |
| 80 ↗ | (On Diff #21366) | Typo |
| 82 ↗ | (On Diff #21366) | Typo |
| 83 ↗ | (On Diff #21366) | Typo |
| 93 ↗ | (On Diff #21366) | Should we be including this on iOS? Will it hurt performance at all? |
| native/chat/typeahead-tooltip.react.js | ||
|---|---|---|
| 6 ↗ | (On Diff #21414) | This should be included from react-native |
| 38–49 ↗ | (On Diff #21414) | This code looks familiar - we probably have something similar on web. Is it a good idea to extract the common part from them? |
| 60 ↗ | (On Diff #21414) | How does this behave when usernames are really long? |
| 114 ↗ | (On Diff #21414) | How were these values determined? Are they correct for different resolutions / pixel densities? |
| native/chat/typeahead-tooltip.react.js | ||
|---|---|---|
| 38–49 ↗ | (On Diff #21414) | You're right. I've created commit 4.5/n and refactored it to seperate common function, so we can use it here |
| 60 ↗ | (On Diff #21414) | It's fine. It puts ellipsis at the end and hides overflow. |
| 114 ↗ | (On Diff #21414) | Experimentally. It was related to chat-input-bar.react.js container: {
backgroundColor: 'listBackground',
paddingLeft: Platform.OS === 'android' ? 10 : 5,
}and https://github.com/facebook/react-native/issues/4240 |