You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "dmytrohoi (via GitHub)" <gi...@apache.org> on 2025/01/10 16:34:29 UTC
[PR] fix(popover): refactor toggle and close handlers for controlled components [superset]
dmytrohoi opened a new pull request, #31792:
URL: https://github.com/apache/superset/pull/31792
### SUMMARY
Fixes #25592
Added clearing of popoverLabel and hasCustomLabel states. Merged handlers that works with toggle/close from props and locally implemented in component to work with new column selectors and edit existing ones.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TBD
### TESTING INSTRUCTIONS
1. On the chart table agregattion edit page
2. Add new column with custom title
3. Open new popover to add column
4. Check that column name in popover is default ('My Column')
### ADDITIONAL INFORMATION
- [x] Has associated issue: #25592
- [ ] Required feature flags:
- [x] 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(popover): refactor toggle and close handlers for controlled components [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #31792:
URL: https://github.com/apache/superset/pull/31792#discussion_r1910638350
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopoverTrigger.tsx:
##########
@@ -72,26 +72,33 @@ const ColumnSelectPopoverTrigger = ({
setPopoverLabel(initialPopoverLabel);
}, [initialPopoverLabel, popoverVisible]);
- const togglePopover = useCallback((visible: boolean) => {
- setPopoverVisible(visible);
- }, []);
+ const handleTogglePopover = useCallback(
+ (visible: boolean) => {
+ if (isControlledComponent) {
+ props.togglePopover!(visible);
+ } else {
+ setPopoverVisible(visible);
+ }
- const closePopover = useCallback(() => {
- setPopoverVisible(false);
- }, []);
+ if (!visible) {
+ setPopoverLabel(initialPopoverLabel);
+ setHasCustomLabel(false);
+ }
+ },
+ [initialPopoverLabel, isControlledComponent, props.togglePopover],
+ );
+
+ const handleClosePopover = useCallback(() => {
+ if (isControlledComponent) {
+ props.closePopover!();
+ } else {
+ setPopoverVisible(false);
+ }
+ setPopoverLabel(initialPopoverLabel);
+ setHasCustomLabel(false);
+ }, [initialPopoverLabel, isControlledComponent, props.closePopover]);
- const { visible, handleTogglePopover, handleClosePopover } =
- isControlledComponent
- ? {
- visible: props.visible,
- handleTogglePopover: props.togglePopover!,
- handleClosePopover: props.closePopover!,
- }
- : {
- visible: popoverVisible,
- handleTogglePopover: togglePopover,
- handleClosePopover: closePopover,
- };
+ const visible = isControlledComponent ? props.visible! : popoverVisible;
Review Comment:
### Unsafe visibility state access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Non-null assertion is used on props.visible without ensuring it exists for controlled components.
###### Why this matters
This could lead to runtime errors if the visible prop is not provided when the component is used in controlled mode.
###### Suggested change ∙ *Feature Preview*
Add a validation check or provide a default value:
```typescript
const visible = isControlledComponent ? props.visible ?? false : popoverVisible;
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:ffc64cdf-788f-49b5-92bd-59887d1fe76b -->
--
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(popover): refactor toggle and close handlers for controlled components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31792:
URL: https://github.com/apache/superset/pull/31792#issuecomment-3180893115
Converted to draft... this seems to be in need of tests since it looks like it adds some complications. If you can add that, we can review it when ready :)
--
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(popover): refactor toggle and close handlers for controlled components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #31792: fix(popover): refactor toggle and close handlers for controlled components
URL: https://github.com/apache/superset/pull/31792
--
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(popover): refactor toggle and close handlers for controlled components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31792:
URL: https://github.com/apache/superset/pull/31792#issuecomment-3644388549
Closing this stale draft PR. It has been open for about 11 months and inactive for 4 months, and still requires tests per the label.
@dmytrohoi if you're still interested in this fix, please feel free to open a fresh PR based on current `master`.
Thanks for your contribution!
--
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