korbit-ai[bot] commented on code in PR #32384:
URL: https://github.com/apache/superset/pull/32384#discussion_r2010734451
##########
superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx:
##########
@@ -216,7 +216,10 @@ class Markdown extends PureComponent {
handleChangeFocus(nextFocus) {
const nextFocused = !!nextFocus;
- const nextEditMode = nextFocused ? 'edit' : 'preview';
+ const nextEditMode = 'preview';
+ if(nextFocused){
+ nextEditMode = 'edit';
+ }
Review Comment:
### Constant Reassignment Error <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code attempts to reassign a constant variable 'nextEditMode', which will
throw a runtime error.
###### Why this matters
This will cause the application to crash when trying to focus the markdown
component, preventing users from entering edit mode.
###### Suggested change ∙ *Feature Preview*
Use let instead of const for the nextEditMode variable:
```javascript
let nextEditMode = 'preview';
if(nextFocused){
nextEditMode = 'edit';
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04b17b86-1005-4549-880c-8e3b905754d8/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04b17b86-1005-4549-880c-8e3b905754d8?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04b17b86-1005-4549-880c-8e3b905754d8?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04b17b86-1005-4549-880c-8e3b905754d8?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04b17b86-1005-4549-880c-8e3b905754d8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6cc754f8-bcf8-4107-a586-60519e41e824 -->
[](6cc754f8-bcf8-4107-a586-60519e41e824)
##########
superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx:
##########
@@ -295,15 +298,17 @@ class Markdown extends PureComponent {
const { hasError } = this.state;
return (
- <SafeMarkdown
- source={
- hasError
- ? MARKDOWN_ERROR_MESSAGE
- : this.state.markdownSource || MARKDOWN_PLACE_HOLDER
- }
- htmlSanitization={this.props.htmlSanitization}
- htmlSchemaOverrides={this.props.htmlSchemaOverrides}
- />
+ <div style={{ pointerEvents: 'none' }}>
Review Comment:
### Unexplained Inline Style <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Hard-coded style object in JSX markup without explanation of its purpose.
###### Why this matters
Inline styles without context make it difficult to understand the intent and
can lead to maintenance issues when the purpose isn't clear.
###### Suggested change ∙ *Feature Preview*
Either extract to a styled component with a descriptive name or add a
comment explaining the purpose:
```javascript
// Prevent interaction with markdown content in preview mode
<div style={{ pointerEvents: 'none' }}>
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac1c097c-c652-4460-9664-9933aba5d2c1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac1c097c-c652-4460-9664-9933aba5d2c1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac1c097c-c652-4460-9664-9933aba5d2c1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac1c097c-c652-4460-9664-9933aba5d2c1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac1c097c-c652-4460-9664-9933aba5d2c1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ed54f0e3-6c47-460c-bcac-5743c6b0e609 -->
[](ed54f0e3-6c47-460c-bcac-5743c6b0e609)
##########
superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx:
##########
@@ -115,10 +115,12 @@ export default class WithPopoverMenu extends
PureComponent<
onChangeFocus: null,
menuItems: [],
isFocused: false,
- shouldFocus: (event: any, container: ShouldFocusContainer) =>
- container?.contains(event.target) ||
- event.target.id === 'menu-item' ||
- event.target.parentNode?.id === 'menu-item',
+ shouldFocus: (event: any, container: ShouldFocusContainer) =>{
+ 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:
### Inconsistent formatting in shouldFocus function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The shouldFocus function has inconsistent spacing and formatting, making it
harder to read. Return statements are placed immediately after conditions
without proper spacing.
###### Why this matters
Poor code formatting reduces scanability and makes code review and
maintenance more time-consuming.
###### Suggested change ∙ *Feature Preview*
```typescript
shouldFocus: (event: any, container: ShouldFocusContainer) => {
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;
},
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a5e99f7-eca8-4c9a-9988-437e9f5d51f2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a5e99f7-eca8-4c9a-9988-437e9f5d51f2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a5e99f7-eca8-4c9a-9988-437e9f5d51f2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a5e99f7-eca8-4c9a-9988-437e9f5d51f2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a5e99f7-eca8-4c9a-9988-437e9f5d51f2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4352f1a9-1062-4d1d-8f8f-9b365016a0bc -->
[](4352f1a9-1062-4d1d-8f8f-9b365016a0bc)
--
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
