gooroodev commented on PR #19717:
URL: https://github.com/apache/echarts/pull/19717#issuecomment-2160346813
Appreciate the mention, @Manviel!
| 🐞Mistake | 🤪Typo | 🚨Security | 🚀Performance | 💪Best Practices |
📖Readability | ❓Others |
|-----------|--------|-------------|---------------|------------------|--------------|----------|
| 1 | 0 | 1 | 1 | 2 | 2
| 0 |
### Summary of Incoming Changes
1. ➕ Added functions `calculateArrowOffset` and `getColorClassName` to
improve modularity and readability.
2. ➕ Introduced CSS class-based styling for tooltips and markers to comply
with strict Content Security Policy (CSP) styles.
3. ➕ Added browser environment to ESLint configuration.
4. ➕ Created `basic.css` file to house common styles.
5. ➕ Introduced utility functions `clearContent`, `setTextContent`, and
`appendArrow` to manage tooltip content.
6. ➖ Removed inline styles and replaced them with CSS classes.
7. 📖 Improved readability by refactoring inline styles into CSS classes.
8. 🚨 Added CSP meta tag to test HTML files.
9. 🚀 Optimized some functions by reducing inline style manipulations.
### Identified Issues
| ID | Type | Details
| Severity | Confidence |
|-----|----------------|-----------------------------------------------------------------------------------------------|----------|------------|
| 1 | 🚨Security | Inline styles removed but some classes like
`tooltip-arrow-offset-*` can lead to dynamic class injection | 🟠Medium |
🟠Medium |
| 2 | 🚀Performance | The function `createElementFromHTML` uses
`DOMParser` which can be replaced with `innerHTML` for better performance |
🟡Low | 🟡Low |
| 3 | 💪Best Practices| The `clearContent` function could be optimized by
setting `innerHTML` to an empty string. | 🟡Low | 🟡Low |
| 4 | 💪Best Practices| The function `appendArrow` creates a new `div`
element every time, which might be unnecessary. | 🟡Low | 🟡Low |
| 5 | 📖Readability | The `calculateArrowOffset` function's formula is
complex and could use inline comments. | 🟡Low | 🟡Low |
| 6 | 📖Readability | The `getColorClassName` function could have a more
descriptive name. | 🟡Low | 🟡Low |
### Code Snippets for Fixes
1. **Security Issue Fix (Dynamic Class Injection)**:
```typescript
// src/component/tooltip/TooltipHTMLContent.ts
function getColorClassName(color: ZRColor) {
const colorValue = convertToColorString(color);
return btoa(colorValue); // Base64 encoding to avoid injection
}
```
2. **Performance Optimization (DOMParser)**:
```typescript
// src/component/tooltip/tooltipMarkup.ts
function createElementFromHTML(htmlString: string) {
const tempDiv = document.createElement('div');
tempDiv.innerHTML = htmlString.trim();
return tempDiv.firstChild;
}
```
3. **Best Practices (clearContent Optimization)**:
```typescript
// src/component/tooltip/TooltipHTMLContent.ts
function clearContent(el: HTMLElement) {
el.innerHTML = '';
}
```
4. **Best Practices (appendArrow Optimization)**:
```typescript
// src/component/tooltip/TooltipHTMLContent.ts
function appendArrow(el: HTMLElement, arrow: string) {
if (!arrow) {
return;
}
el.innerHTML += arrow;
}
```
5. **Readability Improvement (Inline Comments)**:
```typescript
// src/component/tooltip/TooltipHTMLContent.ts
function calculateArrowOffset(rotatedWH: number, borderWidth: number,
arrowWH: number) {
// Calculate the offset of the arrow based on the rotated width and
border width
return Math.round(
(
((rotatedWH - Math.SQRT2 * borderWidth) / 2
+ Math.SQRT2 * borderWidth
- (rotatedWH - arrowWH) / 2)
* 100
) / 100
);
}
```
6. **Readability Improvement (Function Naming)**:
```typescript
// src/component/tooltip/TooltipHTMLContent.ts
function generateColorClassName(color: ZRColor) {
const colorValue = convertToColorString(color);
return colorValue.replace(/[^a-zA-Z0-9]/g, '');
}
```
### General Review
The incoming changes significantly improve the codebase by introducing
modular functions and replacing inline styles with CSS classes, which enhances
compliance with CSP. However, there are minor issues related to security,
performance, and readability that need attention. The code quality is generally
good, but a few optimizations and better naming conventions can further improve
it.
--
I **only** arrive when I am mentioned and asked to review the pull request.
React or reply to keep the discussion going!
--
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]
