korbit-ai[bot] commented on code in PR #32384:
URL: https://github.com/apache/superset/pull/32384#discussion_r1970870900
##########
superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx:
##########
@@ -156,15 +156,18 @@ export default class WithPopoverMenu extends
PureComponent<
if (!this.props.editMode) {
return;
}
- const {
- onChangeFocus,
- shouldFocus: shouldFocusFunc,
- disableClick,
- } = this.props;
- const shouldFocus = shouldFocusFunc(event, this.container);
+ // Allow the event to propagate to the Markdown component
+ const { shouldFocus } = this.props;
+ const shouldFocusComponent = shouldFocus(event, this.container);
- if (!disableClick && shouldFocus && !this.state.isFocused) {
+ if (shouldFocusComponent) {
+ event.stopPropagation();
+ }
Review Comment:
### Incorrect Event Propagation Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The event.stopPropagation() is called when shouldFocusComponent is true,
which contradicts the comment 'Allow the event to propagate to the Markdown
component' and the intended behavior.
###### Why this matters
This will prevent the click event from reaching the Markdown component when
we actually want it to be editable, potentially breaking the main feature this
PR aims to implement.
###### Suggested change ā *Feature Preview*
The event.stopPropagation() should be removed or the logic should be
inverted depending on the exact desired behavior. If we want to allow
propagation to Markdown, simply remove these lines:
```typescript
if (shouldFocusComponent) {
event.stopPropagation();
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2c8c0d61-4803-471d-8ae9-605d79ff190f?suggestedFixEnabled=true)
š¬ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:69ad7fc3-2542-48ae-821c-86c137b5a055 -->
##########
superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx:
##########
@@ -382,6 +386,14 @@ class Markdown extends PureComponent {
ref={dragSourceRef}
className="dashboard-component
dashboard-component-chart-holder"
data-test="dashboard-component-chart-holder"
+ role="button"
+ tabIndex="0"
+ onClick={() => {
+ if (editMode) {
+ this.handleChangeFocus(true);
+ this.handleChangeEditorMode('edit');
+ }
+ }}
Review Comment:
### Redundant State Update in Click Handler <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Redundant call to handleChangeEditorMode('edit') since
handleChangeFocus(true) already triggers edit mode
###### Why this matters
The redundant call creates unnecessary state updates and could potentially
cause race conditions or unexpected behavior in the component's state management
###### Suggested change ā *Feature Preview*
Remove the redundant call to handleChangeEditorMode and only use
handleChangeFocus:
```javascript
onClick={() => {
if (editMode) {
this.handleChangeFocus(true);
}
}}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/61cc874b-5fde-4a63-b073-2118a932a7f2?suggestedFixEnabled=true)
š¬ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:59db4550-f29f-4df5-a7fb-7ae37f326749 -->
--
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
