Fix text selection on Web doesn't work when passing markdownStyle without useMemo#547
Fix text selection on Web doesn't work when passing markdownStyle without useMemo#547SzymczakJ wants to merge 8 commits intoExpensify:mainfrom
Conversation
tomekzaw
left a comment
tomekzaw
left a comment
There was a problem hiding this comment.
Also, does the selection work correctly when changing markdownStyle?
| function deepEqualMarkdownStyles(markdownStyle1: MarkdownStyle, markdownStyle2: PartialMarkdownStyle) { | ||
| const keys1 = Object.keys(markdownStyle1) as Array<keyof MarkdownStyle>; | ||
| const keys2 = Object.keys(markdownStyle2) as Array<keyof MarkdownStyle>; | ||
|
|
||
| if (keys1.length !== keys2.length) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Since all values of markdownStyle are serializable, why can't we just compare JSON.stringify(markdownStyle1) === JSON.stringify(markdownStyle2)?
There was a problem hiding this comment.
Don't we want to treat objects like {color: '#FF0000', backgroundColor: '#FF0000'} and {backgroundColor: '#FF0000', color: '#FF0000'} as equal? That's the only reason I didn't use JSON.stringify, because it maintains order. We could use JSON.stringify, but then some logically equal objects(but with changed order of props) would come out as not equal. WDYT?
src/styleUtils.ts
Outdated
| export type {PartialMarkdownStyle}; | ||
|
|
||
| export {mergeMarkdownStyleWithDefault, parseStringWithUnitToNumber}; | ||
| export {mergeMarkdownStyleWithDefault, parseStringWithUnitToNumber, deepEqualMarkdownStyles as deepCompareMarkdownStyles}; |
There was a problem hiding this comment.
Why do we export deepEqualMarkdownStyles with a different name deepCompareMarkdownStyles?
|
Maybe we should write some tests that would cover this case? @tomekzaw |
|
Definitely yes |
SzymczakJ
force-pushed
the
@szymczak/fix-text-selection
branch
from
1302bf2 to
c89eeea
Compare
|
Can be closed, has been fixed in #684 |
4 participants
Details
markdownStyleobject was compared by reference and was causing extra useMemo rerenders, which removed text selection. This PR makesMarkdownTextInput.webcomponent comparesmarkdownStyleobject deeply and fixes this bug.Related Issues
GH_LINK
#544
Manual Tests
On web example, try selecting text and verify it works.
Linked PRs