-
Notifications
You must be signed in to change notification settings - Fork 6
SCAL-274111 Add hideTagFilterChips option to AppEmbed for cleaner UI #324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
Summary of ChangesHello @shivam-kumar-ts, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new hideTagFilterChips option to BaseViewConfig, which simplifies hiding tag-related UI elements by automatically managing hiddenListColumns and hiddenActions. The implementation is functionally correct. My feedback focuses on improving code conciseness and maintainability by refactoring duplicated logic and fixing a minor formatting issue in the JSDoc.
src/embed/ts-embed.ts
Outdated
| const finalHiddenListColumns = [...(this.viewConfig.hiddenListColumns || [])]; | ||
| if (this.viewConfig.hideTagFilterChips) { | ||
| if (!finalHiddenListColumns.includes(ListPageColumns.Tags)) { | ||
| finalHiddenListColumns.push(ListPageColumns.Tags); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code can be made more concise by using a Set to handle the uniqueness of elements, and conditional spreading to add the new tag. This approach is cleaner and avoids nested if statements. A similar pattern can be applied to the logic for finalHiddenActions as well to reduce code duplication.
const finalHiddenListColumns = [...new Set([
...(this.viewConfig.hiddenListColumns || []),
...(this.viewConfig.hideTagFilterChips ? [ListPageColumns.Tags] : []),
])];
src/embed/ts-embed.ts
Outdated
| const finalHiddenActions = [...(hiddenActions ?? [])]; | ||
| if (hideTagFilterChips) { | ||
| if (!finalHiddenActions.includes(Action.ManageTags)) { | ||
| finalHiddenActions.push(Action.ManageTags); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the logic for hiddenListColumns, this block can be simplified. Using a Set with conditional spreading provides a more concise and readable way to ensure Action.ManageTags is added to the hiddenActions array without duplicates.
const finalHiddenActions = [...new Set([
...(hiddenActions ?? []),
...(hideTagFilterChips ? [Action.ManageTags] : []),
])];
src/types.ts
Outdated
| /** | ||
| * Hide tag filter chips that appear when content is filtered by tags. | ||
| * When enabled, this automatically: | ||
| * - Hides tag filter indicators/chips from the UI | ||
| * - Adds Action.ManageTags to hiddenActions (if not already present) | ||
| * - Adds ListPageColumns.Tags to hiddenListColumns (if not already present) | ||
| * | ||
| * This provides a clean interface without tag-related UI elements. | ||
| * | ||
| * Supported embed types: `AppEmbed` | ||
| * @version SDK: 1.42.1 | ThoughtSpot: 10.14.0.cl | ||
| * @example | ||
| * ```js | ||
| * // Simple usage - automatically hides all tag-related UI | ||
| * const embed = new AppEmbed('#tsEmbed', { | ||
| * ... // other embed view config | ||
| * tag: 'Retail Sales', | ||
| * hideTagFilterChips: true, // This is all you need! | ||
| * }); | ||
| * | ||
| * // No need to manually specify: | ||
| * // hiddenActions: [Action.ManageTags], | ||
| * // hiddenListColumns: [ListPageColumns.Tags] | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc for the new hideTagFilterChips property is indented incorrectly. It should be aligned with the other properties in the BaseViewConfig interface for consistency.
/**
* Hide tag filter chips that appear when content is filtered by tags.
* When enabled, this automatically:
* - Hides tag filter indicators/chips from the UI
* - Adds Action.ManageTags to hiddenActions (if not already present)
* - Adds ListPageColumns.Tags to hiddenListColumns (if not already present)
*
* This provides a clean interface without tag-related UI elements.
*
* Supported embed types: `AppEmbed`
* @version SDK: 1.42.1 | ThoughtSpot: 10.14.0.cl
* @example
* ```js
* // Simple usage - automatically hides all tag-related UI
* const embed = new AppEmbed('#tsEmbed', {
* ... // other embed view config
* tag: 'Retail Sales',
* hideTagFilterChips: true, // This is all you need!
* });
*
* // No need to manually specify:
* // hiddenActions: [Action.ManageTags],
* // hiddenListColumns: [ListPageColumns.Tags]
* ```
*/
commit: |
42356c3 to
aeeb08d
Compare
af5068a to
183ef0c
Compare
src/embed/app.ts
Outdated
| * This provides a clean interface without tag-related UI elements. | ||
| * | ||
| * Supported embed types: `AppEmbed` | ||
| * @version SDK: 1.42.1 | ThoughtSpot: 10.14.0.cl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughtspot version would be 10.15.0.cl
src/embed/app.ts
Outdated
| * This provides a clean interface without tag-related UI elements. | ||
| * | ||
| * Supported embed types: `AppEmbed` | ||
| * @version SDK: 1.42.1 | ThoughtSpot: 10.15.0.cl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SDK version is incorrect.
Shivam while writing the SDK version, map SDK version like this -
For 10.13.0.cl, we will be having 1.42.0
For 10.14.0.cl, we will be having 1.43.0
So for 10.15.0.cl, we will be having 1.44.0 as the SDK version. And if the changes are only in SDK, then add SDK version as next minor version for example - 1.42.1 is already published so next minor version is 1.42.2 so add that as the SDK version and then publish the new version
c7b53ea to
46fdf79
Compare
src/embed/app.ts
Outdated
| * This provides a clean interface without tag-related UI elements. | ||
| * | ||
| * Supported embed types: `AppEmbed` | ||
| * @version SDK: 1.42.2 | ThoughtSpot: 10.15.0.cl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SDK version will be 1.44.0
src/embed/app.ts
Outdated
| * hideTagFilterChips: true, // This is all you need! | ||
| * }); | ||
| * | ||
| * // No need to manually specify: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to mention this
src/embed/app.ts
Outdated
| * Hide tag filter chips that appear when content is filtered by tags. | ||
| * When enabled, this automatically: | ||
| * - Hides tag filter indicators/chips from the UI | ||
| * - Adds Action.ManageTags to hiddenActions (if not already present) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not doing this right? why have we added this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this AppViewConfig will do this things, so added as a reference for the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
d5fa94a to
ec7c7fb
Compare
src/embed/app.ts
Outdated
| params[Param.IsUnifiedSearchExperienceEnabled] = isUnifiedSearchExperienceEnabled; | ||
| params[Param.CoverAndFilterOptionInPDF] = !!coverAndFilterOptionInPDF; | ||
| params[Param.LiveboardXLSXCSVDownload] = !!liveboardXLSXCSVDownload; | ||
| params[Param.HideTagFilterChips] = hideTagFilterChips; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add it by default. It will unnecessary increase the length of the url. Check if it is not undefined then only add it as part of query params. Look at other places how we have added query params optionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
e98108a to
6226a26
Compare
393f415 to
75ede10
Compare
75ede10 to
0142412
Compare
|
SonarQube Quality Gate
|








Uh oh!
There was an error while loading. Please reload this page.