100pah commented on code in PR #20977:
URL: https://github.com/apache/echarts/pull/20977#discussion_r2093276035
##########
src/label/labelStyle.ts:
##########
@@ -395,15 +395,18 @@ function setTextStyleCommon(
let richResult: TextStyleProps['rich'];
if (richItemNames) {
richResult = {};
+ const richInheritPlainLabel =
textStyleModel.get('richInheritPlainLabel') !== false;
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 && textStyleModel);
Review Comment:
A boolean is not supposed to input to `.getModel(xxx, boolean)`,
at present the `textStyleModel` is declared as `Model`, that is effectively
`Madel<any>`, and the TS type of
`textStyleModel.get('richInheritPlainLabel')` is `any`, that make it pass
the ts check, but not quite correct.
I think it can be strict to:
```ts
const richTextStyle = textStyleModel.getModel(['rich', name],
richInheritPlainLabel ? textStyleModel : undefined);
```
##########
src/label/labelStyle.ts:
##########
@@ -395,15 +395,18 @@ function setTextStyleCommon(
let richResult: TextStyleProps['rich'];
if (richItemNames) {
richResult = {};
+ const richInheritPlainLabel =
textStyleModel.get('richInheritPlainLabel') !== false;
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 && textStyleModel);
// In rich, never `disableBox`.
Review Comment:
Just some thoughts:
Do we need to simplify it to only support `richInheritPlainLabel` on the
global model?
(I'm not sure)
+ pros:
+ I guess the most common usage of `richInheritPlainLabel` is when users
upgrade to echarts v6, and find that some text style is broken. They can simply
set `richInheritPlainLabel: false` on the global option to revert it, without
being bothered by debugging. But if they have decided to not to use
`richInheritPlainLabel: false` and fix the broken style one by one, they can
also just modify the text styles directly, but not necessarily set
`richInheritPlainLabel: false` on some specific style option (such as,
`axisLable`, `series[i].label`, ...).
+ If define `richInheritPlainLabel` on the global model, that can be
declared in `types.ts` #`ECUnitOption`. On the contrary, if supporting
`richInheritPlainLabel` every text style, it seems not easy to make a uniform
TS declaration. The `textStyleModel` in this function has been declared as
`Model<any>`, that makes `textStyleModel.get('richInheritPlainLabel')` return
an `any` type, and pass TS check by chance, but not quite correct.
+ cons:
+ `ecModel` is not necessarily exist in this function, tho in most cases
it exists. Is it appropriate to just ignore the cases that `ecModel` does not
exist? I mean `if (ecModel) { ecModel.get('richInheritPlainLabel') } else { /*
just treat it as richInheritPlainLabel: true */ }`.
+ Is it possible that it is hard to fix the style and have to rely on
`richInheritPlainLabel: true`?
But since no ideal approach, it's fine to use the current implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
