Add support for Ceros Flex embeds#1934
Conversation
DikshitaKhandke
left a comment
There was a problem hiding this comment.
@copilot-robot prerelease
|
@copilot-robot prerelease |
|
Hi, @fennen529, there are no changed packages to publish. |
| return ( | ||
| a.type === "script" && | ||
| typeof a.attributes.src === "string" && | ||
| /(?:^|\/)embed_v\d+\.js(?:[?#].*)?$/i.test(a.attributes.src) |
There was a problem hiding this comment.
this test doesn't seem specific enough, especially for the open-source atjson package that might be used in any arbitrary context. It should at least have the hostname in the test
There was a problem hiding this comment.
Good call. I tightened the Flex detection so it now checks the documented Ceros host and path (assets.ceros.site/js/embed.v*.js) instead of matching any similar filename. I also updated the fixture to use the documented script URL and added a negative test to confirm a non-Ceros host with a similar script name does not get parsed as Flex.
| */ | ||
| url: string; | ||
|
|
||
| /** |
There was a problem hiding this comment.
I think this should be a union type, like
class CerosEmbed extends BlockAnnotation<
{cerosType?: "studio"; url: string; aspectRatio: number}
| {cerosType: "flex"; url: string; experienceUrl: string; scriptUrl: string; width: string; height: string}
>There was a problem hiding this comment.
Agreed. I moved CerosEmbed to a discriminated union so Studio and Flex now have distinct shapes instead of one wide object with loosely related optional fields. I kept url as the canonical experience URL for both variants and dropped experienceUrl, since it was redundant with the renderer logic.
tim-evans
left a comment
There was a problem hiding this comment.
I have some questions and would like some further information on valid values! Thanks 😄
| /** | ||
| * The script URL required to bootstrap a Flex embed. | ||
| */ | ||
| scriptUrl?: string; |
There was a problem hiding this comment.
The script URL was previously provided by upstream clients to ensure that the creative URL and the script could be assigned separately. Is there a reason for these to be bundled together?
There was a problem hiding this comment.
I separated the concerns more clearly. url is now the canonical experience URL, and scriptUrl remains a separate Flex-specific field, so they are no longer bundled conceptually in the model.
| /** | ||
| * The URL to the Flex experience when represented separately | ||
| * from the canonical `url` field. | ||
| */ | ||
| experienceUrl?: string; |
There was a problem hiding this comment.
Seeing experienceUrl || url in code below indicates that this field is perhaps redundant
There was a problem hiding this comment.
Agreed. I removed experienceUrl and standardized on url as the single canonical Ceros experience URL. The renderer no longer falls back between the two.
| "data-embed-width": embed.attributes.width, | ||
| "data-embed-height": embed.attributes.height, |
There was a problem hiding this comment.
Could you provide some documentation on valid values for width and height?
There was a problem hiding this comment.
In our upstream systems width and height cannot be auto.
There was a problem hiding this comment.
Yes, I added documentation for those fields in the annotation type. Based on the Ceros Flex docs, width is typically a percentage like 100%, and height is either auto for full-height embeds or a fixed pixel value like 800px for scrolling embeds.
|
From the Ceros documentation:
There are different scripts used for each:
There are notes about adding Aspect ratios could still be used as a storage mechanism for this annotation, alongside a boolean that controls whether the embed full screen or scrolling, per the documentation 😄 |
Agreed on the script distinction and the accessibility point. I updated the implementation to use the documented Studio/Flex script URLs and to preserve Flex data-title for accessibility. For the model itself, I kept this change closer to the documented embed shape by storing the explicit Flex fields rather than normalizing into aspect ratio + fullscreen/scrolling. That normalization could still be a good follow-up, but I kept this pass focused on correctness and round-tripping the documented embed format. |
| */ | ||
| anchorName?: string; | ||
| }> { | ||
| scriptUrl?: string; |
There was a problem hiding this comment.
Please remove the script url from the annotation. This should be managed upstream by any client using this to ensure that they don't include the script twice on one page, for example.
| * either `auto` for a full-height embed or a fixed pixel value | ||
| * such as `800px` for a scrolling embed. | ||
| */ | ||
| height?: string; |
There was a problem hiding this comment.
auto I'm fairly sure will not be compatible with our upstream parsers.
Either verify that this works upstream as a valid value for a width or make this an enum that specifies behavior (a suggestion is full-height | scrolling)
There was a problem hiding this comment.
Updated this as per slack comments
DikshitaKhandke
left a comment
There was a problem hiding this comment.
Hey @tim-evans Can you please review this PR as well.
tim-evans
left a comment
There was a problem hiding this comment.
One requested change to the types
| import { BlockAnnotation } from "@atjson/document"; | ||
|
|
||
| export class CerosEmbed extends BlockAnnotation<{ | ||
| type SharedCerosEmbedAttributes = { |
There was a problem hiding this comment.
Please inline these as it makes types opaque to any clients using this package
DikshitaKhandke
left a comment
There was a problem hiding this comment.
@copilot-robot prerelease
tim-evans
left a comment
There was a problem hiding this comment.
Thanks for adapting to our rounds of reviews 😄
|
@copilot-robot prerelease |
|
Hi, @a-rena, there are no changed packages to publish. |
Ticket : https://cnissues.atlassian.net/browse/ATC-6999