You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2025/08/21 12:14:17 UTC

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

msyavuz opened a new pull request, #34790:
URL: https://github.com/apache/superset/pull/34790

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   Applied filters color is changed on a recent theming pr, this one fixes it and also implements a scrollable applied filters display.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   Before:
   
   <img width="1122" height="565" alt="image" src="https://github.com/user-attachments/assets/14476697-3b95-4628-8a42-1e79a6b7bf43" />
   
   After:
   <img width="1080" height="553" alt="image" src="https://github.com/user-attachments/assets/f2d23e43-9882-4004-8269-c24d0b7788ae" />
   
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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-unsubscribe@superset.apache.org

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


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

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

Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz merged PR #34790:
URL: https://github.com/apache/superset/pull/34790


-- 
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-unsubscribe@superset.apache.org

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


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

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

Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #34790:
URL: https://github.com/apache/superset/pull/34790#discussion_r2293238668


##########
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:
   List component from antd provides scrolling functionality



-- 
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-unsubscribe@superset.apache.org

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


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

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

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #34790:
URL: https://github.com/apache/superset/pull/34790#issuecomment-3210352751

   Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
   
   > Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
   
   
   
   [](76244b68-10a5-41c2-a252-a0b957ee4955)


-- 
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-unsubscribe@superset.apache.org

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


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

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

Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #34790:
URL: https://github.com/apache/superset/pull/34790#issuecomment-3210354071

   @kasiazjc What do you think about the scrollable container? After which point we should make it scroll instead of grow?


-- 
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-unsubscribe@superset.apache.org

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


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

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

Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #34790:
URL: https://github.com/apache/superset/pull/34790#discussion_r2293240183


##########
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:
   This again the approach antd takes on documentation



-- 
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-unsubscribe@superset.apache.org

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


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

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

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34790:
URL: https://github.com/apache/superset/pull/34790#discussion_r2293240970


##########
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:
   I see, following AntD's patterns makes sense here. The List component likely handles optimizations internally. Let's keep the current implementation unless performance issues arise in practice.



-- 
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-unsubscribe@superset.apache.org

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


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

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

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34790:
URL: https://github.com/apache/superset/pull/34790#discussion_r2293239375


##########
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:
   Understood - since antd List handles scrolling, no explicit overflow handling needed here. Thanks for clarifying.



-- 
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-unsubscribe@superset.apache.org

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


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

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

Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #34790:
URL: https://github.com/apache/superset/pull/34790#issuecomment-3213602559

   > @kasiazjc What do you think about the scrollable container? After which point we should make it scroll instead of grow?
   
   looks great! in general for the popover I think it will be displayed bottom or top of the trigger icon, based on the scroll location of the user right? 
   
   I would just make sure that it always is displayed at the top of the other components (likefilter bar or nav bar) and there is top and bottom margin of 36px so that it doesn't touch the window (browser) edges. If the height is reached enable internal scroll 


-- 
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-unsubscribe@superset.apache.org

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


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

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

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org

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


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