Conversation
Let an embed host choose the appearance of the embedded application without reaching for additionalFlags, the generic escape hatch whose own doc comment warns it is for advanced use only and that we do not publish the supported keys for. The value travels as a query param on the iframe URL, so the embedded application has it before first paint and renders dark with no flash of the light theme. - Add EmbedConfig.darkMode, the appearance for every embed on the page - Add BaseViewConfig.darkMode, so one embed can override that - Add Param.DarkMode and emit it from getBaseQueryParams, the single origin of the query params for every embed type - Resolve the two levels with ??, so that an explicit view-level false overrides an init-level true and only undefined falls through - Omit the param entirely when neither level sets it, keeping the URL short for the hosts that never ask for a dark embed - Cover both config levels, an explicit false and the omitted case Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for a darkMode option, allowing the embedded application to be rendered in dark or light appearance. It adds darkMode configuration options to both EmbedConfig and BaseViewConfig, updates the query parameter generation in TsEmbed, and includes comprehensive unit tests. The review feedback highlights two style guide violations in the JSDoc code examples within src/types.ts: one example is missing a closing }); for an init() call, and another uses invalid placeholder syntax instead of valid JavaScript/TypeScript.
| * init({ | ||
| * ...embedConfig, | ||
| * darkMode: true, | ||
| * }) |
There was a problem hiding this comment.
The code example for init() is missing the closing });. According to the style guide (rule #65), init() calls in examples must be complete to be syntactically valid.
| * init({ | |
| * ...embedConfig, | |
| * darkMode: true, | |
| * }) | |
| init({ | |
| ...embedConfig, | |
| darkMode: true, | |
| }); |
References
- Rule test-exported memb #65:
init()calls in examples must include the closing});. (link)
| * // Replace <EmbedComponent> with embed component name. For example, AppEmbed, SearchEmbed, or LiveboardEmbed | ||
| * const embed = new <EmbedComponent>('#tsEmbed', { | ||
| * ... // other embed view config | ||
| * darkMode: true, | ||
| * }); |
There was a problem hiding this comment.
The code example uses which is not valid JavaScript/TypeScript syntax, violating style guide rule #61. Additionally, the indentation within the code block is inconsistent. Please use a concrete component name for the example and ensure consistent indentation for better readability.
| * // Replace <EmbedComponent> with embed component name. For example, AppEmbed, SearchEmbed, or LiveboardEmbed | |
| * const embed = new <EmbedComponent>('#tsEmbed', { | |
| * ... // other embed view config | |
| * darkMode: true, | |
| * }); | |
| // Replace AppEmbed with a specific embed component like SearchEmbed or LiveboardEmbed | |
| const embed = new AppEmbed('#tsEmbed', { | |
| ... // other embed view config | |
| darkMode: true, | |
| }); |
References
- Rule SCAL-232534 - Added flags to enable disable spotter features #61: Code examples must use syntactically valid JavaScript/TypeScript. (link)
commit: |
Review feedback on the code examples in the darkMode doc comments, against the two style guide rules under Code Example Integrity. - Close the init() example with `});`, per the rule that init() calls in examples must include it - Name a concrete component in the view config example instead of `new <EmbedComponent>(...)`, which is not valid JavaScript - Line up the indentation inside the view config example Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
| * The host application owns the appearance of an embed, so this overrides | ||
| * the appearance preference of the signed-in user and the color scheme of | ||
| * their operating system. Leave it unset to render in light appearance. | ||
| * @version SDK: 1.53.0 | ThoughtSpot Cloud: 26.10.0.cl |
There was a problem hiding this comment.
who this is 26.10.0.cl are we doing mcp?
| * The host application owns the appearance of an embed, so this overrides | ||
| * the appearance preference of the signed-in user and the color scheme of | ||
| * their operating system. Leave it unset to render in light appearance. | ||
| * @version SDK: 1.53.0 | ThoughtSpot Cloud: 26.11.0.cl |
There was a problem hiding this comment.
it will be 54
@version SDK: 1.50.0 | ThoughtSpot Cloud: 26.7.0.cl
This branch has not been deployed
2 participants
Let an embed host choose the appearance of the embedded application without reaching for additionalFlags, the generic escape hatch whose own doc comment warns it is for advanced use only and that we do not publish the supported keys for.
The value travels as a query param on the iframe URL, so the embedded application has it before first paint and renders dark with no flash of the light theme.