Skip to content

Page designer i frame implementation - #129

Open
asad-rafter wants to merge 9 commits into
cloudinary:new_iframefrom
asad-rafter:page-designer-iFrame-implementation
Open

asad-rafter wants to merge 9 commits into
cloudinary:new_iframefrom
asad-rafter:page-designer-iFrame-implementation

Conversation

@asad-rafter

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread cartridges/bm_cloudinary_pd/cartridge/experience/editors/cloudinary/imageForm.js Outdated
Comment thread cartridges/bm_cloudinary_pd/cartridge/experience/editors/cloudinary/videoForm.js Outdated

// Size the container to fill the modal viewport
parentIFrame.getPageInfo(function (info) {
var rem = parseFloat(getComputedStyle(document.documentElement).fontSize) || 16;

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.

@asad-rafter please use Numbaer.parseFloat instead of parseFloat

setTimeout(function () {
try {
var buttons = window.parent.document.querySelectorAll('button');
for (var b = 0; b < buttons.length; b++) {

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.

@asad-rafter please use for-of loop

@@ -0,0 +1,173 @@
/**

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.

@asad-rafter please use optional chain expression, and avoid unicode chars and long separation lines

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.

try {
var ffOptions = JSON.parse(player.formFactorOptions);
var w = window.innerWidth;
var ff = w <= 767 ? 'mobile' : (w <= 1023 ? 'tablet' : 'desktop');

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.

@asad-rafter are these numbers hardcodded in PD? is there any chance the customer would need different breakpoints

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.

background: #fff;
}

.cld-adv-applied {

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.

@asad-rafter where is this class used?

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.

try {
var parsed = JSON.parse(playerOptionsRaw);
var keys = ['autoplay', 'muted', 'loop', 'controls'];
for (var i = 0; i < keys.length; i++) {

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.

@asad-rafter please use for-of loop

var order = ['mobile', 'tablet', 'desktop'];
var ff = getActiveFormFactor();
if (formValues[ff]) return formValues[ff];
for (var i = 0; i < order.length; i++) {

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.

@asad-rafter Please use a for-of loop

var formFactorOrder = ['mobile', 'tablet', 'desktop'];
var formFactorImageUrls = {};
var lastResolvedUrl = null;
for (var i = 0; i < formFactorOrder.length; i++) {

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.

@asad-rafter Please use a for-of loop

var order = ['mobile', 'tablet', 'desktop'];
var ff = getActiveFormFactor();
if (formValues[ff]) return formValues[ff];
for (var i = 0; i < order.length; i++) {

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.

@asad-rafter Please use a for-of loop@asad-rafter Please use a for-of loop

var ffOrder = ['mobile', 'tablet', 'desktop'];
var ffOptions = {};

for (var fi = 0; fi < ffOrder.length; fi++) {

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.

@asad-rafter Please use a for-of loop

// ── New per-form-factor format ────────────────────────────────────────
if (isNewFormat(val)) {
var entry = resolveEntry(val.formValues);
if (!entry || !entry.asset) return viewmodel;

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.

@asad-rafter please use ?.


var asset = entry.asset;
var publicId = asset.public_id;
var cloudName = currentSite.getCustomPreferenceValue('CloudinaryPageDesignerCloudName');

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.

@asad-rafter cname and cloudName are used also out this if-block. Move the definitions one level higher

for (var i = 0; i < formFactorOrder.length; i++) {
var formFactor = formFactorOrder[i];
var formFactorEntry = val.formValues[formFactor];
if (formFactorEntry && formFactorEntry.asset) {

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.

@asad-rafter please use ?.

}

// ── Legacy format ─────────────────────────────────────────────────────
if (context.content[editorId] && context.content[editorId].imageUrl) {

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.

@asad-rafter please use ?.


// ── Legacy format ─────────────────────────────────────────────────────
if (context.content[editorId] && context.content[editorId].imageUrl) {
var cname = currentSite.getCustomPreferenceValue('CloudinaryPageDesignerCNAME');

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.

@asad-rafter would become redundant once the code is fixed with previous comment

@yuval-cloudinary

Copy link
Copy Markdown
Contributor

@asad-rafter please use util functions for the code shared by both widgets, so it won't be written twic:

The exact same block — { mobile: 767, tablet: 1023 } + JSON.parse + two typeof === 'number' guards + try/catch — appears in:

│ File │ Line │
│ bm/.../editors/cloudinary/imageForm.js │ ~13 │
│ bm/.../editors/cloudinary/videoForm.js │ ~33 │
│ bm/static/.../imageFormWidget.js toFormFactor │ ~68 │
│ bm/static/.../videoFormWidget.js toFormFactor │ ~74 │
│ int/.../assets/mediaLibrary.js preRender │ ~236 │
│ int/.../assets/mediaLibraryVideo.js preRender │ ~427 │
│ int/static/js/cloudinaryVideos.js resolvePlayerOptions │ ~14 │

@yuval-cloudinary

Copy link
Copy Markdown
Contributor

@asad-rafter please check if we can remove
experience/editors/cloudinary/imageForm.js and experience/editors/cloudinary/videoForm.js

@asad-rafter

Copy link
Copy Markdown
Contributor Author

@asad-rafter please check if we can remove experience/editors/cloudinary/imageForm.js and experience/editors/cloudinary/videoForm.js

Checked — they can't be removed. SFCC Page Designer requires a dedicated server-side JS init file per editor (referenced via the JSON descriptor). The two files serve distinct purposes: imageForm.js wires up mediaPicker + studioWidget, while videoForm.js wires up mediaPicker + advancedConfig and also configures playerOptions. They now share the breakpoints logic via cloudinaryPDUtils, so duplication is gone even though the files themselves remain.


.cld-advanced-btn:disabled,
.cld-advanced-btn[aria-disabled="true"] {
color: #c9c7c5;

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.

@asad-rafter Please use color: #757575 to adhere to accessibility standards

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