korbit-ai[bot] commented on code in PR #33196:
URL: https://github.com/apache/superset/pull/33196#discussion_r2052828568
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -188,6 +188,47 @@ class BigNumberVis extends
PureComponent<BigNumberVizProps> {
);
}
+ renderSubheader(maxHeight: number) {
+ const { bigNumber, subheader, width, bigNumberFallback } = this.props;
+ let fontSize = 0;
+
+ const NO_DATA_OR_HASNT_LANDED = t(
+ 'No data after filtering or data is NULL for the latest time record',
+ );
+ const NO_DATA = t(
+ 'Try applying different filters or ensuring your datasource has data',
+ );
+ let text = subheader;
+ if (bigNumber === null) {
+ text = bigNumberFallback ? NO_DATA : NO_DATA_OR_HASNT_LANDED;
+ }
Review Comment:
### Subheader override on null data <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The condition overrides the subheader text with error messages when
bigNumber is null, even when there's an explicitly provided subheader.
###### Why this matters
A user might want to display a custom subheader message even when there's no
data, but this logic always replaces it with error messages, reducing
flexibility and potentially confusing users who expect their custom message to
appear.
###### Suggested change ∙ *Feature Preview*
Modify the condition to only set error messages when no subheader is
provided:
```typescript
if (bigNumber === null && !subheader) {
text = bigNumberFallback ? NO_DATA : NO_DATA_OR_HASNT_LANDED;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a5ce15-1e83-4366-a338-18f72cfcbab0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a5ce15-1e83-4366-a338-18f72cfcbab0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a5ce15-1e83-4366-a338-18f72cfcbab0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a5ce15-1e83-4366-a338-18f72cfcbab0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a5ce15-1e83-4366-a338-18f72cfcbab0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a727ee19-d8cc-4828-94b0-0a4da621c437 -->
[](a727ee19-d8cc-4828-94b0-0a4da621c437)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -188,6 +188,47 @@
);
}
+ renderSubheader(maxHeight: number) {
+ const { bigNumber, subheader, width, bigNumberFallback } = this.props;
+ let fontSize = 0;
Review Comment:
### Redundant fontSize initialization <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The fontSize variable is unnecessarily initialized to 0 since it's always
overwritten when text exists.
###### Why this matters
While not causing immediate issues, this initialization creates unnecessary
code and could mislead developers into thinking the fontSize might be used when
text is falsy.
###### Suggested change ∙ *Feature Preview*
Remove the unnecessary initialization:
```typescript
const { bigNumber, subheader, width, bigNumberFallback } = this.props;
let fontSize;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd861c53-b3e3-4c22-a387-76af043e4faa/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd861c53-b3e3-4c22-a387-76af043e4faa?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd861c53-b3e3-4c22-a387-76af043e4faa?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd861c53-b3e3-4c22-a387-76af043e4faa?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd861c53-b3e3-4c22-a387-76af043e4faa)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4213071d-fb49-493a-9d9f-81dbe1848803 -->
[](4213071d-fb49-493a-9d9f-81dbe1848803)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -188,6 +188,47 @@
);
}
+ renderSubheader(maxHeight: number) {
+ const { bigNumber, subheader, width, bigNumberFallback } = this.props;
+ let fontSize = 0;
+
+ const NO_DATA_OR_HASNT_LANDED = t(
+ 'No data after filtering or data is NULL for the latest time record',
+ );
+ const NO_DATA = t(
+ 'Try applying different filters or ensuring your datasource has data',
+ );
Review Comment:
### Localized Constants Defined in Method Scope <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Error message constants are defined inside the renderSubheader method
instead of at the component or module level.
###### Why this matters
Defining constants inside a method makes them recreated on every render and
makes them inaccessible to other methods that might need the same messages.
###### Suggested change ∙ *Feature Preview*
Move the constants outside the component:
```typescript
const NO_DATA_OR_HASNT_LANDED = t(
'No data after filtering or data is NULL for the latest time record',
);
const NO_DATA = t(
'Try applying different filters or ensuring your datasource has data',
);
class BigNumberVis extends PureComponent<BigNumberVizProps> {
// ... rest of the code
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/657a4f44-07e5-4f65-ac0c-da59bad2a506/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/657a4f44-07e5-4f65-ac0c-da59bad2a506?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/657a4f44-07e5-4f65-ac0c-da59bad2a506?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/657a4f44-07e5-4f65-ac0c-da59bad2a506?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/657a4f44-07e5-4f65-ac0c-da59bad2a506)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:50a632fd-fb84-4a5d-8ccf-0aa30248eb53 -->
[](50a632fd-fb84-4a5d-8ccf-0aa30248eb53)
--
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]
