fix(Axistip): add supports the formatter and fix the tips display abnormally #248
Conversation
WalkthroughA new mechanism for handling axis label formatting has been added to the axis tip feature. Formatter functions for axis labels are now collected and stored by axis type. When displaying tooltips on mouse movement, the code checks for a custom formatter for the relevant axis and uses it to format the tooltip content if available. If no formatter is set, it defaults to displaying the raw value. The rest of the tooltip behavior, including positioning and event triggering, remains unchanged. Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/feature/axistip/index.jsOops! Something went wrong! :( ESLint: 8.56.0 ReferenceError: require is not defined in ES module scope, you can use import instead Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/feature/axistip/index.js (1)
93-104: 💡 Verification agent🧩 Analysis chain
Verify event listener cleanup on chart recreation
The current implementation adds event listeners each time
axistipis called, but doesn't remove previous ones. This could lead to memory leaks and duplicate tooltips if the same chart instance is reused.
🏁 Script executed:
#!/bin/bash # Check if there are any calls to remove event listeners echo "Checking for event listener cleanup..." rg -A 3 -B 3 "echartsIns\.off|removeEventListener" src/ echo "Checking if axistip function is called multiple times for the same chart instance..." rg -A 3 -B 3 "axistip\(" src/ | grep -v "function axistip"Length of output: 5760
Add cleanup for prior ‘mousemove’ listener before rebinding
Before registering the new handler you should remove any existing listener to prevent duplicates or leaks:
• File:
src/feature/axistip/index.js
Insert a call toechartsIns.off('mousemove')(or pass the same callback reference) immediately beforeechartsIns.on('mousemove', …).Suggested diff:
tipContainer.style.opacity = '0'; - echartsIns.on('mousemove', (param) => { + // remove any previously bound mousemove handler + echartsIns.off('mousemove'); + // define handler so it can be unbound later if needed + const handleMouseMove = (param) => { let type = param.componentType; if (param.name) { tipContainer.textContent = param.name; @@ if (axisType.indexOf(param.componentType) !== -1) { setPosition(tipContainer, echartsDom, param); } - }); + }; + echartsIns.on('mousemove', handleMouseMove);This ensures each chart recreation only has one active tooltip listener.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/feature/axistip/index.js(3 hunks)
🔇 Additional comments (1)
src/feature/axistip/index.js (1)
86-86: LGTM - Guard clause to skip processing falsy axis typesThis is a good addition that ensures we only process axis types that are explicitly enabled.
| @@ -15,12 +15,17 @@ import { isArray, isBoolean } from '../../util/type'; | |||
| const distanceX = 16; // 鼠标与气泡框之间的左右偏移量 | |||
| const distanceY = 24; // 鼠标与气泡框之间的上偏移量 | |||
| const axisType = ['xAxis', 'yAxis']; | |||
| let textFormatter = {}; | |||
There was a problem hiding this comment.
Consider scoping textFormatter to avoid state persistence between calls
The textFormatter variable is declared at module scope using let, which means its state will persist between multiple calls to the axistip function. This could lead to unexpected behavior if formatters from previous chart instances are applied to new charts.
-let textFormatter = {};Move the declaration inside the axistip function to reset it on each call:
function axistip(echartsDom, echartsIns, eChartOption, axistip) {
if (!axistip) return;
+ let textFormatter = {};
if (isBoolean(axistip)) {
axistip = {};
axisType.forEach(item => {
axistip[item] = true;
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let textFormatter = {}; | |
| // --- Remove this module‑scope line: | |
| //- let textFormatter = {}; | |
| // Move into the function to reset on each call: | |
| function axistip(echartsDom, echartsIns, eChartOption, axistip) { | |
| if (!axistip) return; | |
| let textFormatter = {}; | |
| if (isBoolean(axistip)) { | |
| axistip = {}; | |
| axisType.forEach(item => { | |
| axistip[item] = true; | |
| }); | |
| } | |
| // …rest of axistip implementation that uses textFormatter… | |
| } |
| let type = param.componentType; | ||
| if(param.name){ | ||
| tipContainer.textContent = param.name; | ||
| }else{ | ||
| tipContainer.textContent = textFormatter[type] ? textFormatter[type](param.value) : param.value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for formatter functions
The current implementation directly calls the formatter function without any error handling. If the formatter throws an error, it could break the entire chart's functionality.
-tipContainer.textContent = textFormatter[type] ? textFormatter[type](param.value) : param.value;
+try {
+ tipContainer.textContent = textFormatter[type] ? textFormatter[type](param.value) : param.value;
+} catch (error) {
+ console.error('Error applying axis formatter:', error);
+ tipContainer.textContent = param.value;
+}Also, maintain consistent use of spaces in conditional expressions:
-if(param.name){
+if (param.name) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let type = param.componentType; | |
| if(param.name){ | |
| tipContainer.textContent = param.name; | |
| }else{ | |
| tipContainer.textContent = textFormatter[type] ? textFormatter[type](param.value) : param.value; | |
| } | |
| let type = param.componentType; | |
| if (param.name) { | |
| tipContainer.textContent = param.name; | |
| } else { | |
| try { | |
| tipContainer.textContent = textFormatter[type] | |
| ? textFormatter[type](param.value) | |
| : param.value; | |
| } catch (error) { | |
| console.error('Error applying axis formatter:', error); | |
| tipContainer.textContent = param.value; | |
| } | |
| } |
| textFormatter[type] = undefined; | ||
| if(subitem.axisLabel?.formatter){ | ||
| textFormatter[type] = subitem.axisLabel.formatter | ||
| } |
There was a problem hiding this comment.
Add formatter handling for non-array axis configuration
The formatter is only extracted when eChartOption[type] is an array, but not when it's a single object. This is inconsistent with how triggerEvent is handled.
} else {
+ textFormatter[type] = undefined;
+ if(eChartOption[type].axisLabel?.formatter){
+ textFormatter[type] = eChartOption[type].axisLabel.formatter
+ }
eChartOption[type].triggerEvent = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| textFormatter[type] = undefined; | |
| if(subitem.axisLabel?.formatter){ | |
| textFormatter[type] = subitem.axisLabel.formatter | |
| } | |
| } else { | |
| textFormatter[type] = undefined; | |
| if (eChartOption[type].axisLabel?.formatter) { | |
| textFormatter[type] = eChartOption[type].axisLabel.formatter | |
| } | |
| eChartOption[type].triggerEvent = true; | |
| } |
lingdan33
changed the title
lingdan33
changed the title
Summary by CodeRabbit
New Features
Bug Fixes