Re: [PR] fix(Chart): Calculate chart height correctly [superset]

[via GitHub]

korbit-ai[bot] commented on code in PR #34224:
URL: https://github.com/apache/superset/pull/34224#discussion_r2216438897


##########
superset-frontend/src/dashboard/components/gridComponents/Chart.jsx:
##########
@@ -213,11 +213,15 @@
 
   const getHeaderHeight = useCallback(() => {
     if (headerRef.current) {
-      const computedStyle = getComputedStyle(
+      const computedMarginBottom = getComputedStyle(
         headerRef.current,
       ).getPropertyValue('margin-bottom');
-      const marginBottom = parseInt(computedStyle, 10) || 0;
-      return headerRef.current.offsetHeight + marginBottom;
+      const marginBottom = parseInt(computedMarginBottom, 10) || 0;
+      const computedHeight = getComputedStyle(
+        headerRef.current,
+      ).getPropertyValue('height');
+      const height = parseInt(computedHeight, 10) || DEFAULT_HEADER_HEIGHT;

Review Comment:
   ### Invalid CSS Height Parsing <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The fallback to DEFAULT_HEADER_HEIGHT when parseInt fails could be incorrect 
if there's a valid non-pixel CSS height value (e.g., 'auto', '100%', 'inherit')
   
   
   ###### Why this matters
   This could lead to incorrect chart height calculations when header heights 
are specified using non-pixel units, potentially causing rendering issues or 
visual glitches.
   
   ###### Suggested change ∙ *Feature Preview*
   Replace with a more robust height calculation that considers non-pixel 
values:
   ```javascript
   const height = computedHeight === 'auto' || computedHeight.endsWith('%')
     ? headerRef.current.offsetHeight
     : parseInt(computedHeight, 10) || DEFAULT_HEADER_HEIGHT;
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76087eaf-02a5-412e-8996-786b4adac4a0/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76087eaf-02a5-412e-8996-786b4adac4a0?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76087eaf-02a5-412e-8996-786b4adac4a0?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76087eaf-02a5-412e-8996-786b4adac4a0?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76087eaf-02a5-412e-8996-786b4adac4a0)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:c1a18160-b86f-4e52-a217-43ff0f0581d0 -->
   
   
   [](c1a18160-b86f-4e52-a217-43ff0f0581d0)



##########
superset-frontend/src/dashboard/components/gridComponents/Chart.jsx:
##########
@@ -213,11 +213,15 @@ const Chart = props => {
 
   const getHeaderHeight = useCallback(() => {
     if (headerRef.current) {
-      const computedStyle = getComputedStyle(
+      const computedMarginBottom = getComputedStyle(
         headerRef.current,
       ).getPropertyValue('margin-bottom');
-      const marginBottom = parseInt(computedStyle, 10) || 0;
-      return headerRef.current.offsetHeight + marginBottom;
+      const marginBottom = parseInt(computedMarginBottom, 10) || 0;
+      const computedHeight = getComputedStyle(
+        headerRef.current,
+      ).getPropertyValue('height');
+      const height = parseInt(computedHeight, 10) || DEFAULT_HEADER_HEIGHT;
+      return height + marginBottom;
     }
     return DEFAULT_HEADER_HEIGHT;
   }, [headerRef]);

Review Comment:
   ### Duplicated Style Computation Logic <sub>![category 
Design](https://img.shields.io/badge/Design-0d9488)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The getHeaderHeight function contains repeated calls to getComputedStyle 
which could be extracted into a reusable helper function.
   
   
   ###### Why this matters
   Having duplicate getComputedStyle logic makes the code less maintainable and 
violates the DRY principle. Future changes would need to be made in multiple 
places.
   
   ###### Suggested change ∙ *Feature Preview*
   Extract the getComputedStyle logic into a reusable helper function:
   ```javascript
   const getComputedStyleValue = (element, property) => {
     const value = getComputedStyle(element).getPropertyValue(property);
     return parseInt(value, 10) || 0;
   };
   
   const getHeaderHeight = useCallback(() => {
     if (headerRef.current) {
       const marginBottom = getComputedStyleValue(headerRef.current, 
'margin-bottom');
       const height = getComputedStyleValue(headerRef.current, 'height') || 
DEFAULT_HEADER_HEIGHT;
       return height + marginBottom;
     }
     return DEFAULT_HEADER_HEIGHT;
   }, [headerRef]);
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/20a06e0b-7f30-4c46-8d22-7e6614b646e9/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/20a06e0b-7f30-4c46-8d22-7e6614b646e9?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/20a06e0b-7f30-4c46-8d22-7e6614b646e9?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/20a06e0b-7f30-4c46-8d22-7e6614b646e9?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/20a06e0b-7f30-4c46-8d22-7e6614b646e9)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:e84f216e-b993-4233-93cc-f6cf6f3ba480 -->
   
   
   [](e84f216e-b993-4233-93cc-f6cf6f3ba480)



-- 
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: notifications-unsubscr...@superset.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org