Uh oh!
There was an error while loading. Please reload this page.
Do not serialize text attributes that are not used later - #42703
Do not serialize text attributes that are not used later#42703jakub-trzebiatowski wants to merge 1 commit into
Conversation
| public static TextAttributeProps fromMapBuffer(MapBuffer props) { | ||
| TextAttributeProps result = new TextAttributeProps(); | ||
| // TODO T83483191: Review constants that are not being set! |
There was a problem hiding this comment.
We're deserializing text attributes of a fragment. Some attributes are not handled on the fragment level but apply to the whole Text element. I believe there's not much more to investigate here.
There was a problem hiding this comment.
The fact that we currently don't do what this comment describes is blocking me.
I'd like to...
- Split
TextAttributes(C++) toTextAttributes(<Text>-level properties) andFragmentAttributes(fragment-level properties) - Refactor
TextAttributeProps(Java) toFragmentAttributes
Currently, one is a subset of another; not modeling them separately is at most a code smell.
I'd like them to be two slightly different sets (sharing a great common subset), so not modeling them separately is a problem.
There was a problem hiding this comment.
yes, this doesn't seem correct, thanks for cleaning up
jakub-trzebiatowski
commented
Jan 29, 2024
@mdvacca I think I will need your eyes on this one. Please see this thread. |
analysis-bot
commented
Jan 29, 2024
Base commit: cfc0ba0 |
jakub-trzebiatowski
commented
Feb 5, 2024
@mdvacca What do you think? Is this a good way forward? |
facebook-github-bot
commented
Feb 6, 2024
@mdvacca has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| public static TextAttributeProps fromMapBuffer(MapBuffer props) { | ||
| TextAttributeProps result = new TextAttributeProps(); | ||
| // TODO T83483191: Review constants that are not being set! |
There was a problem hiding this comment.
yes, this doesn't seem correct, thanks for cleaning up
jakub-trzebiatowski
commented
Feb 15, 2024
@mdvacca Let me know if I can help with making the internal build succeed |
react-native-bot
commented
Aug 14, 2024
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Summary:
Do not serialize text attributes that are not used later
Changelog:
[INTERNAL] [REMOVED] - Do not serialize text attributes that are not used later
Test Plan: