This diff adds the chatMentionCandidates object to all functions that return textMessageRules in rules.react.js, and includes this object as a parameter in all occurrences of these functions.
Depends on D8871.
Details
Details
Before testing, please apply the entire native stack.
- Check if mentions are rendered correctly (see D8845).
- Check if mention content is not replaced to default text (@[[id:default text]]) when pending sidebar is created.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/markdown/rules.react.js | ||
|---|---|---|
| 406–416 ↗ | (On Diff #30691) | You might be wondering, if this function is almost the same as getDefaultTextMessageRules in D8851, then it should be moved to lib. We could introduce a new function which accepts textMessageRules as an argument and executes this logic. However I see some issues: what to do with defaultTextMessageRules? Since textMessageRules returns different values on native and web what should be returned when we override this variable? Solution to this would be to pass defaultTextMessageRules as an argument but I feel like this will make getDefaultTextMessageRules function more complicated. Another issue is what if textMessageRules will have additional argument only on web? That's why I've decided to duplicate this code in two places. If arguments in textMessageRules will change, then flow will probably indicate that textMessageRules must be modified in getDefaultTextMessageRules. If you still think that this should be refactored, please let me know! |
Comment Actions
It might make sense to introduce a new hook that:
- Get the candidates using useThreadChatMentionCandidates
- Computes the rules using getDefaultTextMessageRules
- Memoizes them using memo
| native/markdown/rules.react.js | ||
|---|---|---|
| 409–415 ↗ | (On Diff #30691) | You can early return to reduce indentation |
Comment Actions
@tomek's improvement makes sense, but I'm not applying them right now because I don't believe it will bring significant improvements. If there will be any performance issues during testing, it might be worth wrapping default rules with memo.
Do we really need one-offs here? Why can't we find an existing color family that works with our designs?
We need to push the team (@tedchang in particular) to use existing families (and update them as necessary) instead of hacking around inconsistencies like this. When we take this sort of approach of picking colors on a per-screen basis, we fail to maintain stylistic / design consistency throughout the app