Skip to content

SCAL-334772 Extract full-height support into a shared module - #641

Open
shivam-kumar-ts wants to merge 2 commits into
mainfrom
SCAL-334772
Open

SCAL-334772 Extract full-height support into a shared module#641
shivam-kumar-ts wants to merge 2 commits into
mainfrom
SCAL-334772

Conversation

@shivam-kumar-ts

Copy link
Copy Markdown
Contributor

What

The full-height feature was implemented twice, verbatim — once in LiveboardEmbed and once in AppEmbed. This PR extracts it into a single FullHeightController in src/full-height.ts, backed by a shared FullHeightViewConfig type, and deletes ~470 lines of duplication from the embed classes.

No change to the public API surface: fullHeight, minimumHeight, defaultHeight, lazyLoadingForFullHeight, lazyLoadingMargin and enableScrollableContainerLazyLoading are accepted by exactly the same embeds as before, with the same semantics.

Why

Both embeds carried identical copies of sendFullHeightLazyLoadData, requestVisibleEmbedCoordinatesHandler, updateIFrameHeight, embedIframeCenter, setIframeHeightForNonEmbedLiveboard, registerLazyLoadEvents and unregisterLazyLoadEvents, plus the same three state fields, the same constructor wiring and the same query-param block. The liveboardRelatedRoutes list was duplicated character-for-character. A third copy of the viewport math lived in TsEmbed.getIframeCenter().

Every fix to this feature had to land in two or three places, and the two copies had already drifted (AppEmbed silently ignored defaultHeight).

How

  • src/full-height.ts (new) — FullHeightController owns all full-height state, event handlers, query params and listener lifecycle. It talks to the embed through a 4-method FullHeightEmbedHost contract (getIframe / setFrameHeight / on / trigger), so it needs nothing from the embed class hierarchy. Inert unless fullHeight is enabled.
  • src/types.tsFullHeightViewConfig holds the six full-height props; LiveboardViewConfig and AppViewConfig extend it and drop their duplicated declarations.
  • src/utils.tscalculateElementCenter(element) generalises the viewport math that was inlined in TsEmbed.
  • src/embed/{app,liveboard}.ts — reduced to constructing the controller and forwarding four lifecycle calls.
  • src/embed/ts-embed.tsgetIframeCenter() is now a thin delegate to calculateElementCenter.

The defaultHeight mutation that used to happen as a side effect of getEmbedParamsObject() is now a minimumHeight getter derived from the view config, removing the ordering dependency between param building and height events.

File
src/embed/app.ts +19 / −226
src/embed/liveboard.ts +26 / −248
src/embed/ts-embed.ts +2 / −25
src/full-height.ts +284 (new)
src/types.ts +135
src/utils.ts +38

Behavior change ⚠️

AppEmbed now honours defaultHeight. It previously resolved minimumHeight || 500 and ignored defaultHeight entirely; LiveboardEmbed resolved minimumHeight || defaultHeight || 500. The shared getter applies the Liveboard rule to both.

This is additive and non-breaking — passing defaultHeight to AppEmbed was a TypeScript excess-property error before, so no typed consumer can regress. minimumHeight still wins where both are set.

@shivam-kumar-ts
shivam-kumar-ts requested a review from a team as a code owner August 26, 2026 05:14
@pkg-pr-new

pkg-pr-new Bot commented Aug 26, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@thoughtspot/visual-embed-sdk@641

commit: 3390f7b

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 refactors the full-height and lazy-loading logic for AppEmbed and LiveboardEmbed by extracting it into a new, dedicated FullHeightController class. This centralizes height negotiation, query parameter additions, and viewport listeners for lazy loading, improving code modularity and testability. Corresponding tests have been updated, and a new test suite src/full-height.spec.ts has been added. Additionally, the inline iframe center calculation in TsEmbed has been replaced with a helper function calculateElementCenter in utils.ts.

Feedback is provided regarding a style guide violation in src/types.ts, where the non-standard @type tag is used in TSDoc comments.

Comment thread src/types.ts
@shivam-kumar-ts
shivam-kumar-ts force-pushed the SCAL-334772 branch 2 times, most recently from 9477f62 to 1fa4c7f Compare August 26, 2026 15:20
Comment thread src/types.ts Outdated
Comment thread src/utils.ts
Comment thread src/embed/liveboard.ts Outdated
Comment thread src/embed/ts-embed.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants