SCAL-313693 Create an visual embed sdk flag for controlling the new e…#531
SCAL-313693 Create an visual embed sdk flag for controlling the new e…#531yinstardev wants to merge 1 commit into
Conversation
commit: |
![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 the enableConnectionNewExperience flag to the AppEmbed configuration, allowing users to enable a new connection experience. The changes include updates to the AppViewConfig interface, the AppEmbed class, and the Param enum, along with corresponding unit tests. The review feedback identifies several violations of the style guide: the @version tag in the JSDoc needs to be corrected to match the canonical SDK-to-Cloud version mapping, and the boolean parameter should not have a default value of false in destructuring. Instead, it should remain undefined and be omitted from the URL when not explicitly provided to prevent URL bloat. Consequently, the unit tests should also be updated to reflect that the parameter is omitted by default.
| * This flag is used to enable the new connection experience for AppEmbed. | ||
| * | ||
| * Supported embed types: `AppEmbed` | ||
| * @version SDK: 1.50.0 | ThoughtSpot Cloud: 26.8.0.cl |
There was a problem hiding this comment.
The version pairing for SDK 1.50.0 should map to ThoughtSpot Cloud 26.7.0.cl according to the canonical mapping in the style guide.
| * @version SDK: 1.50.0 | ThoughtSpot Cloud: 26.8.0.cl | |
| * @version SDK: 1.50.0 | ThoughtSpot Cloud: 26.7.0.cl |
| hideApplicationSwitcher, | ||
| hideOrgSwitcher, | ||
| enableSearchAssist, | ||
| enableConnectionNewExperience = false, |
There was a problem hiding this comment.
Boolean parameters should not have a default value of false in destructuring. They should remain undefined to allow for omission from the URL when not explicitly set by the user, reducing URL bloat.
| enableConnectionNewExperience = false, | |
| enableConnectionNewExperience, |
References
- Do not use = false as a default in destructuring. Leave the value as undefined when not set, and only add the param to the request when the caller explicitly provides a value (!== undefined). (link)
| params[Param.EnableSearchAssist] = enableSearchAssist; | ||
| } | ||
|
|
||
| params[Param.EnableConnectionNewExperience] = enableConnectionNewExperience; |
There was a problem hiding this comment.
This parameter should only be added to the request when the caller explicitly provides a value. Redundant false values should be omitted from the URL.
if (enableConnectionNewExperience !== undefined) {
params[Param.EnableConnectionNewExperience] = enableConnectionNewExperience;
}References
- Omit the param entirely when the value is not explicitly provided. Only add the param to the request when the caller explicitly provides a value (!== undefined). (link)
| await executeAfterWait(() => { | ||
| expectUrlMatchesWithParams( | ||
| getIFrameSrc(), | ||
| `http://${thoughtSpotHost}/?embedApp=true&primaryNavHidden=true&profileAndHelpInNavBarHidden=false&enableConnectionNewExperience=false${defaultParams}${defaultParamsPost}#/home`, |
There was a problem hiding this comment.
Since the parameter should be omitted from the URL when not explicitly provided (default behavior), the test expectation should be updated to reflect that enableConnectionNewExperience=false is no longer present in the default URL.
| `http://${thoughtSpotHost}/?embedApp=true&primaryNavHidden=true&profileAndHelpInNavBarHidden=false&enableConnectionNewExperience=false${defaultParams}${defaultParamsPost}#/home`, | |
| `http://${thoughtSpotHost}/?embedApp=true&primaryNavHidden=true&profileAndHelpInNavBarHidden=false${defaultParams}${defaultParamsPost}#/home`, |
References
- Omit the param entirely when the value is not explicitly provided to reduce URL/query string size unnecessarily. (link)
2 participants
…xperience in TSE