Skip to content
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

Task/web 3112/preview height fix #242

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

PraveenCTzen
Copy link
Contributor

No description provided.

src/util/tr.js Outdated
if (displayObj.preview && displayObj['custom-editor']) {
iframe.sandbox = 'allow-scripts allow-popups allow-popups-to-escape-sandbox'
/* eslint-disable no-cond-assign */
var properties = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use let please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declare this variable outside the if scope to make it available on line 538

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/util/tr.js Outdated
var styleContent, cssBlock, cssMatch

// Extract style blocks
while (styleContent = styleRegex.exec(html)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did not understand this code, why are we assigning things in a while loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is used to extract all the '<style>' block from html.
The while loop runs as long as styleRegex.exec(html) finds a match, processing each style block one at a time.

src/util/tr.js Outdated
Comment on lines 511 to 513
while (cssBlock = ctClassRegex.exec(styleContent[1])) {
// Extract property-value pairs
while (cssMatch = cssRegex.exec(cssBlock[1])) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not look right to me, can you explain what are you doing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 'ctClassRegex' is used to find the CSS blocks for .CT_Box or .CT_Banner within the style content.
  • 'cssRegex' extracts individual CSS properties from the matched CSS block (like height: 500px;).

The two nested while loops allow the script to:
Identify and iterate over each relevant CSS block (.CT_Box or .CT_Banner).
Extract and process each CSS property within those blocks, looking specifically for the height property.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PraveenCTzen This can be optimized, add terminating conditions as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KambleSonam Can you help clarify the type of terminating statements we should add here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant at what condition will the loop break?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the condition of the while loop. Please review it.

/* eslint-disable no-cond-assign */
var properties = {}
const styleRegex = /<style[^>]*>([^<]*)<\/style>/g
const ctClassRegex = /\.CT_(?:Box|Banner)\s*{([^}]*)}/g // Regex to match either .CT_Box or .CT_Banner
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about interstitial ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its working fine!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are still console errors, can we add handling for interstitial as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These errors are related to click tracking, which we are unable to perform in a sandboxed iframe.

src/util/tr.js Outdated
if (displayObj.preview && displayObj['custom-editor']) {
iframe.sandbox = 'allow-scripts allow-popups allow-popups-to-escape-sandbox'
/* eslint-disable no-cond-assign */
var properties = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declare this variable outside the if scope to make it available on line 538

src/util/tr.js Outdated
const properties = {}
if (displayObj.preview && displayObj['custom-editor']) {
iframe.sandbox = 'allow-scripts allow-popups allow-popups-to-escape-sandbox'
/* eslint-disable no-cond-assign */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should always avoid using eslint disable unless we absolutely need to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored it

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.

3 participants