-
Notifications
You must be signed in to change notification settings - Fork 19.8k
fix(label): fix label rich style does not inherit the plain label style #20977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking "Sign up for GitHub", you agree to our terms of service and privacy statement. We'll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(label): fix label rich style does not inherit the plain label style #20977
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -395,15 +395,25 @@ function setTextStyleCommon( | |
| let richResult: TextStyleProps['rich']; | ||
| if (richItemNames) { | ||
| richResult = {}; | ||
| const richInheritPlainLabelOptionName = 'richInheritPlainLabel' as const; | ||
| const richInheritPlainLabel = retrieve2( | ||
| textStyleModel.get(richInheritPlainLabelOptionName), | ||
| ecModel && ecModel.get(richInheritPlainLabelOptionName) | ||
| ); | ||
| for (const name in richItemNames) { | ||
| if (richItemNames.hasOwnProperty(name)) { | ||
| // Cascade is supported in rich. | ||
| const richTextStyle = textStyleModel.getModel(['rich', name]); | ||
| const richTextStyle = textStyleModel.getModel( | ||
| ['rich', name], | ||
| richInheritPlainLabel !== false ? textStyleModel : void 0 | ||
| ); | ||
| // In rich, never `disableBox`. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just some thoughts: Do we need to simplify it to only support
But since no ideal approach, it's also fine to use the current implementation.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the sake of migration, I think supporting the global
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found that after this changing, The root cause of that Error is related to zr incorrect impl (I'm fixing it.), but not caused by this PR. Through this case, I realized that:
Thus should we only conservatively only allow inheritance in Moreover, I found that it's not a correct way to implements the inheritance by const richTextStyle = textStyleModel.getModel(
['rich', name], richInheritPlainLabel !== false ? textStyleModel : void 0 ); See Regarding impl, because in another branch I encountered this issue, I just try to fix that in this way: |
||
| // FIXME: consider `label: {formatter: '{a|xx}', color: 'blue', rich: {a: {}}}`, | ||
| // consider `label: {formatter: '{a|xx}', color: 'blue', rich: {a: {}}}`, | ||
| // the default color `'blue'` will not be adopted if no color declared in `rich`. | ||
| // That might confuses users. So probably we should put `textStyleModel` as the | ||
| // root ancestor of the `richTextStyle`. But that would be a break change. | ||
| // Since v6, the rich style inherits plain label by default | ||
| // but this behavior can be disabled by setting `richInheritPlainLabel` to `false`. | ||
| setTokenTextStyle( | ||
| richResult[name] = {}, richTextStyle, globalTextStyle, opt, isNotNormal, isAttached, false, true | ||
| ); | ||
|
|
||