korbit-ai[bot] commented on code in PR #35336:
URL: https://github.com/apache/superset/pull/35336#discussion_r2389656225
##########
superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.jsx:
##########
@@ -268,6 +269,14 @@ class Markdown extends PureComponent {
}
}
+ shouldFocusMarkdown(event, container) {
+ if (container?.contains(event.target)) return true;
+ if (event.target.id === 'menu-item') return true;
+ if (event.target.parentNode?.id === 'menu-item') return true;
+
+ return false;
+ }
Review Comment:
### Hardcoded ID dependency in focus detection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The shouldFocusMarkdown method uses hardcoded string 'menu-item' for ID
checks, which creates a brittle dependency on specific DOM element IDs that may
not exist or could change.
###### Why this matters
This approach makes the code fragile and prone to breaking if the menu item
structure changes, and it may not correctly identify all relevant menu elements
that should maintain focus.
###### Suggested change ∙ *Feature Preview*
Replace hardcoded ID checks with more robust element detection using CSS
classes, data attributes, or element relationships:
```javascript
shouldFocusMarkdown(event, container) {
if (container?.contains(event.target)) return true;
// Check if target is within a menu context
const menuElement = event.target.closest('[role="menu"], .menu-item,
[data-menu-item]');
if (menuElement) return true;
return false;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/713ce1d8-8ce2-4aa0-abed-eec011e04adf/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/713ce1d8-8ce2-4aa0-abed-eec011e04adf?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/713ce1d8-8ce2-4aa0-abed-eec011e04adf?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/713ce1d8-8ce2-4aa0-abed-eec011e04adf?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/713ce1d8-8ce2-4aa0-abed-eec011e04adf)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:59103f9b-1109-4a36-b787-c896a80f7dcb -->
[](59103f9b-1109-4a36-b787-c896a80f7dcb)
##########
superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx:
##########
@@ -165,6 +173,11 @@ export default class WithPopoverMenu extends PureComponent<
const shouldFocus = shouldFocusFunc(event, this.container);
+ // Only stop propagation if we're actually handling a focus state change
+ if (this.shouldHandleFocusChange(shouldFocus)) {
+ event.stopPropagation();
+ }
Review Comment:
### Event propagation logic may break outside-click detection
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The conditional logic for stopping event propagation may not align with the
intended behavior based on the developer's intent to exit markdown edit mode
when clicking outside.
###### Why this matters
The current logic stops propagation when focus changes are needed, but this
could interfere with outside-click detection mechanisms that rely on event
bubbling to determine if a click occurred outside the component.
###### Suggested change ∙ *Feature Preview*
Consider inverting the logic or removing the stopPropagation call entirely
to ensure outside-click detection works properly:
```typescript
// Allow propagation for outside clicks to be detected properly
if (shouldFocus && !this.state.isFocused) {
// Only stop propagation when focusing, not when unfocusing
event.stopPropagation();
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6111f38-a780-42b7-9955-0e01c6fd1d3c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6111f38-a780-42b7-9955-0e01c6fd1d3c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6111f38-a780-42b7-9955-0e01c6fd1d3c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6111f38-a780-42b7-9955-0e01c6fd1d3c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e6111f38-a780-42b7-9955-0e01c6fd1d3c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:464b960f-00a8-4a34-8c46-069613c073b8 -->
[](464b960f-00a8-4a34-8c46-069613c073b8)
--
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]
