Re: [PR] feat(theming): Support theming on embedding-sdk [superset]

Mon, 05 May 2025 06:40:02 -0700 


korbit-ai[bot] commented on code in PR #33360:
URL: https://github.com/apache/superset/pull/33360#discussion_r2073471964


##########
superset-frontend/src/embedded/index.tsx:
##########
@@ -240,6 +241,21 @@ window.addEventListener('message', function 
embeddedPageInitializer(event) {
     );
     Switchboard.defineMethod('getActiveTabs', embeddedApi.getActiveTabs);
     Switchboard.defineMethod('getDataMask', embeddedApi.getDataMask);
+    Switchboard.defineMethod(
+      'setThemeConfig',
+      (payload: { themeConfig: AnyThemeConfig }) => {
+        const { themeConfig } = payload;
+        log('Received setThemeConfig request:', themeConfig);
+
+        try {
+          themeObject.setConfig(themeConfig);
+          return { success: true, message: 'Theme applied (placeholder)' };

Review Comment:
   ### Placeholder message in production code <sub>![category 
Readability](https://img.shields.io/badge/Readability-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The hardcoded '(placeholder)' string in the success message suggests 
incomplete implementation and reduces code clarity.
   
   ###### Why this matters
   Placeholder messages in production code create uncertainty about whether the 
feature is fully implemented and what the actual message should be.
   
   ###### Suggested change ∙ *Feature Preview*
   ```typescript
   return { success: true, message: 'Theme configuration successfully applied' 
};
   ```
   
   
   ###### 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/0b4ab3f5-38c3-48a9-918e-da9ff8271c57/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b4ab3f5-38c3-48a9-918e-da9ff8271c57?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/0b4ab3f5-38c3-48a9-918e-da9ff8271c57?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/0b4ab3f5-38c3-48a9-918e-da9ff8271c57?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b4ab3f5-38c3-48a9-918e-da9ff8271c57)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:efd1bb61-14ee-4bc4-9d58-da10ff856228 -->
   
   
   [](efd1bb61-14ee-4bc4-9d58-da10ff856228)



##########
superset-embedded-sdk/src/index.ts:
##########
@@ -89,6 +89,7 @@ export type EmbeddedDashboard = {
     callbackFn: ObserveDataMaskCallbackFn,
   ) => void;
   getDataMask: () => Record<string, any>;
+  setThemeConfig: (themeConfig: Record<string, any>) => void;

Review Comment:
   ### Missing setThemeConfig method documentation <sub>![category 
Documentation](https://img.shields.io/badge/Documentation-7c3aed)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The new setThemeConfig method in the EmbeddedDashboard type lacks 
documentation explaining its purpose and impact.
   
   ###### Why this matters
   Without clear documentation, developers may incorrectly use the theme 
configuration or be unaware of its capabilities and limitations.
   
   ###### Suggested change ∙ *Feature Preview*
   /** Sets the theme configuration for the embedded dashboard */ 
setThemeConfig: (themeConfig: Record<string, any>) => void;
   
   
   ###### 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/82437521-1d8a-4645-bbc7-e43c7d69a452/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/82437521-1d8a-4645-bbc7-e43c7d69a452?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/82437521-1d8a-4645-bbc7-e43c7d69a452?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/82437521-1d8a-4645-bbc7-e43c7d69a452?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/82437521-1d8a-4645-bbc7-e43c7d69a452)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:e53aac48-8f75-4804-9c4a-668b7239298f -->
   
   
   [](e53aac48-8f75-4804-9c4a-668b7239298f)



-- 
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]

Reply via email to