Re: [PR] fix(DetailsPanel): Applied filters colors [superset]

Fri, 22 Aug 2025 02:42:34 -0700 


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


##########
superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx:
##########
@@ -103,6 +103,7 @@ export const FilterItem = styled.button`
 
 export const FiltersContainer = styled.div`
   ${({ theme }) => css`
+    max-height: 60vh;

Review Comment:
   ### Missing overflow handling for FiltersContainer height limit 
<sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The fixed maximum height of 60vh for FiltersContainer might truncate content 
without providing a scrolling mechanism.
   
   
   ###### Why this matters
   If filters exceed the 60vh height limit, content will be cut off and 
inaccessible since no overflow behavior is specified.
   
   ###### Suggested change ∙ *Feature Preview*
   Add overflow-y property to enable scrolling when content exceeds the 
max-height:
   ```css
   export const FiltersContainer = styled.div`
     ${({ theme }) => css`
       max-height: 60vh;
       overflow-y: auto;
       margin-top: ${theme.sizeUnit}px;
       &:not(:last-child) {
         padding-bottom: ${theme.sizeUnit * 3}px;
       }
     `}
   `;
   ```
   
   
   ###### 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/69ec505b-8e30-4c93-a6c3-96b268dbafc7/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69ec505b-8e30-4c93-a6c3-96b268dbafc7?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/69ec505b-8e30-4c93-a6c3-96b268dbafc7?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/69ec505b-8e30-4c93-a6c3-96b268dbafc7?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69ec505b-8e30-4c93-a6c3-96b268dbafc7)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:d27b1d72-49fe-4a53-b9ac-11dbe871aec6 -->
   
   
   [](d27b1d72-49fe-4a53-b9ac-11dbe871aec6)



##########
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx:
##########
@@ -158,14 +158,19 @@ const DetailsPanelPopover = ({
               {t('Applied filters (%d)', appliedIndicators.length)}
             </SectionName>
             <FiltersContainer>
-              {appliedIndicators.map(indicator => (
-                <FilterIndicator
-                  ref={el => indicatorRefs.current.push(el)}
-                  key={indicatorKey(indicator)}
-                  indicator={indicator}
-                  onClick={onHighlightFilterSource}
-                />
-              ))}
+              <List
+                dataSource={appliedIndicators}

Review Comment:
   ### Inefficient List Rendering <sub>![category 
Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Using List component with direct array binding can cause unnecessary 
re-renders of all items when any filter changes.
   
   
   ###### Why this matters
   When the appliedIndicators array reference changes, all List items will 
re-render even if individual items haven't changed, impacting performance with 
large filter sets.
   
   ###### Suggested change ∙ *Feature Preview*
   Add memoization to prevent unnecessary re-renders by either using 
React.useMemo for the List or implementing a custom 
shouldComponentUpdate/React.memo with proper comparison:
   
   
   ###### 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/d178ad9c-d1d3-4073-8adc-6d7ee9cc605d/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d178ad9c-d1d3-4073-8adc-6d7ee9cc605d?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/d178ad9c-d1d3-4073-8adc-6d7ee9cc605d?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/d178ad9c-d1d3-4073-8adc-6d7ee9cc605d?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d178ad9c-d1d3-4073-8adc-6d7ee9cc605d)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:5d739414-4d77-4815-af7d-cccf00f244f1 -->
   
   
   [](5d739414-4d77-4815-af7d-cccf00f244f1)



-- 
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]

Reply via email to