-
Notifications
You must be signed in to change notification settings - Fork 169
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
feat: Attributes support for insert script #1386
Conversation
…g to custom component
Editor toast is visible even after saving the rule |
return ruleData; | ||
} | ||
|
||
const ruleDataCopy = _.cloneDeep(ruleData); |
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.
Why do we need a deep copy of rule data? Can you also add a comment here?
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.
RuleData is explicitly read-only (reference to value in store). Updating it directly is possible but leads to very messy code
Added a comment
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.
@nsrCodes I am still not sure you really need to deep clone the ruleData
Looking at this code below 👇 You are again creating a new ruleObject with new rulePairs
![image](https://private-user-images.githubusercontent.com/3108399/303170120-4b30404f-be16-48de-ba72-fae837ec0c29.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1Njg1OTgsIm5iZiI6MTcyNTU2ODI5OCwicGF0aCI6Ii8zMTA4Mzk5LzMwMzE3MDEyMC00YjMwNDA0Zi1iZTE2LTQ4ZGUtYmE3Mi1mYWU4MzdlYzBjMjkucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwNSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDVUMjAzMTM4WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9OTRkZjUxM2U5NDA5NDA4YjkyMzBlY2JlYTQxYmViZjU1ZWRjYzU5N2FkZDYyMGQ3Mzg4NThmYWEzYTRjNTBhYSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.1wLXiqtTGJHLmfV_It8b1IZ6EBIPIiw5_42c_MIckfk)
Just sharing my understanding of ...
operator
![image](https://private-user-images.githubusercontent.com/3108399/303170424-779ae737-df85-4722-9b46-9e4e8234f1ae.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1Njg1OTgsIm5iZiI6MTcyNTU2ODI5OCwicGF0aCI6Ii8zMTA4Mzk5LzMwMzE3MDQyNC03NzlhZTczNy1kZjg1LTQ3MjItOWI0Ni05ZTRlODIzNGYxYWUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwNSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDVUMjAzMTM4WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YjQ0MjFkZDI3MWI3ZjVlMGQwM2IyMjgyNGRmN2YwNjljMDUzYzM3Y2E4NDllNjZjYmZjODUzM2ZiOTFiNmY0YyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.v8nQRfhigho-t-mMJ6kZ6SEaoLDGUHS2tcEOLFPU6fg)
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.
And, Please do not resolve any conversation just after your comment, let me resolve the conversation otherwise I will miss the responses.
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 works for when we only need to change 1 level deep.
Here the whole object immutable, down to every scrip object which is why had to deep clone
...components/features/rules/RuleBuilder/Header/ActionButtons/CreateRuleButton/actions/index.js
Outdated
Show resolved
Hide resolved
const isCompatibleWithAttributesForScriptRule = isFeatureCompatible(FEATURES.SCRIPT_RULE_HTML_BLOCK); | ||
|
||
if (!isCompatibleWithAttributesForScriptRule) { | ||
return ruleData; | ||
} |
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 should be moved into a separate function like checkScriptRuleVersionCompatibility()
function checkcheckScriptRuleVersionCompatibility() {
// Check for Attributes support added in Feb 2024
const isCompatibleForAttribues = isFeatureCompatible(FEATURES.SCRIPT_RULE_ATTRIBUTES_SUPPORT);
if (!isCompatibleForAttribues) return false;
// We can similarly check other compatible things in future
}
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.
doing this just for this check would go agains the current convention across the code where we just use isFeatureCompatible(<feature>)
for all such similar checks
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.
but extracting that to a variable here does seem redundant so replaced that with the function call itself
const isCompatibleWithAttributesForScriptRule = isFeatureCompatible(FEATURES.SCRIPT_RULE_HTML_BLOCK); | ||
|
||
if (!isCompatibleWithAttributesForScriptRule) { | ||
return ruleData; | ||
} | ||
|
||
const ruleDataCopy = _.cloneDeep(ruleData); | ||
const newPairs = []; | ||
|
||
for (const pair of ruleDataCopy.pairs) { | ||
const parsedPair = pair; | ||
const newScripts = []; | ||
|
||
for (const script of pair.scripts) { | ||
let newScript = script; | ||
|
||
const htmlNodeName = getHTMLNodeName(script.type, script.codeType); | ||
|
||
if (script.type === GLOBAL_CONSTANTS.SCRIPT_TYPES.CODE) { | ||
const res = await validateStringForSpecificHTMLNode(script.value, htmlNodeName); | ||
if (!res.isValid) { | ||
return { | ||
error: res.validationError, | ||
success: false, | ||
message: res.errorMessage, | ||
pairId: pair.id, | ||
}; | ||
} | ||
const { attributes, innerText: code } = extractInnerTextAndAttributesFromHTMLString( | ||
script.value, | ||
htmlNodeName | ||
); | ||
newScript = { | ||
...script, | ||
attributes, | ||
value: code, | ||
}; | ||
} else if (script.type === GLOBAL_CONSTANTS.SCRIPT_TYPES.URL) { | ||
const res = await validateStringForSpecificHTMLNode(script.wrapperElement, htmlNodeName); | ||
if (!res.isValid) { | ||
return { | ||
error: res.validationError, | ||
success: false, | ||
message: res.errorMessage, | ||
pairId: pair.id, | ||
}; |
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.
I think we should break down this code into smaller functions. It will also help in writing UTs.
if (doesStringContainHTMLSnippet(code)) { | ||
return { | ||
isValid: false, | ||
error: scriptLogicalErrors.CONTAINS_HTML_CODE, |
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.
Similarly, update the error type CONTAINS_NON_JS_CSS
} | ||
|
||
/* HTML VALIDATORS */ | ||
export const invalidHTMLError = { |
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 should be in CAPS HTML_ERRORS.COULD_NOT_PARSE
* @param {Document} doc Document created from the HTML string | ||
* @returns {boolean} | ||
*/ | ||
function isHTMLString(str, doc) { |
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.
What is the difference between isHTMLString
and doesContainHTMLString
function that you defined above?
I see the code is being duplicated. Can we avoid it?
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.
changed doesContainHTMLString
to isJSOrCSS
cause as you mentioned, that was the intent there
...es/rules/RuleBuilder/Header/ActionButtons/CreateRuleButton/actions/insertScriptValidators.js
Outdated
Show resolved
Hide resolved
...es/rules/RuleBuilder/Header/ActionButtons/CreateRuleButton/actions/insertScriptValidators.js
Outdated
Show resolved
Hide resolved
if (codeType === GLOBAL_CONSTANTS.SCRIPT_CODE_TYPES.JS) { | ||
// else CSS | ||
htmlNode = "script"; | ||
} else if (scriptType === GLOBAL_CONSTANTS.SCRIPT_TYPES.URL) { |
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 should still wrap this under codeType check
export function getHTMLNodeName(scriptType, codeType) { | ||
let htmlNode = ""; | ||
if (codeType === GLOBAL_CONSTANTS.SCRIPT_CODE_TYPES.JS) { | ||
// else CSS |
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 comment isn't placed correctly
return htmlNode; | ||
} | ||
|
||
function extractDomNodeDetailsFromHTMLCodeString(htmlCodeString, nodeName) { |
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.
extractDOMNodeDetails
@@ -17,3 +17,7 @@ | |||
border-bottom-left-radius: var(--border-radius-sm); | |||
border-bottom-right-radius: var(--border-radius-sm); | |||
} | |||
|
|||
.code-editor-small { | |||
max-height: 72px; |
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.
@rohanmathur91 @RuntimeTerror10 -- You can review how we put styles
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.
Please remove this class as this is not being used
}), | ||
[] | ||
); | ||
const isCompatibleWithAttributes = isFeatureCompatible(FEATURES.SCRIPT_RULE_HTML_BLOCK); |
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.
Please rename to FEATURES.SCRIPT_RULE.ATTRIBUTES_SUPPORT because we still don't support HTML block. It sounds confusing
<Col xl="12" span={24}> | ||
<CodeEditor | ||
id={pair.id} | ||
height={75} |
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.
@RuntimeTerror10 @rohanmathur91 Do we specify fixed values like 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.
Yes, the code editor height is resizable by dragging so we need to specify a fixed minimum height for the code editor.
box-shadow: none; | ||
height: 100%; | ||
|
||
align-self: center; |
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.
@RuntimeTerror10 -- I am leaving all the CSS and UI code changes to you.
if (script.type === RQ.SCRIPT_TYPES.URL) { | ||
RQ.ClientUtils.addRemoteJS(script.value, callback); | ||
if (script.attributes) { | ||
RQ.ScriptRuleHandler.addRemoteJSWithAttributes(script.value, script.attributes, callback); |
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 shouldn't create a separate function but instead just pass the attributes and the addRemoteJS function should check if there are attributes then add them otherwise not.
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.
updated this in a separate PR to make it easy for @nafees87n to also review it - #1400
@@ -17,3 +17,7 @@ | |||
border-bottom-left-radius: var(--border-radius-sm); | |||
border-bottom-right-radius: var(--border-radius-sm); | |||
} | |||
|
|||
.code-editor-small { | |||
max-height: 72px; |
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.
Please remove this class as this is not being used
app/src/components/features/rules/RulePairs/Pairs/Rows/RowsMarkup/CustomScriptRow/index.js
Outdated
Show resolved
Hide resolved
app/src/components/features/rules/RulePairs/Pairs/Rows/RowsMarkup/CustomScriptRow/index.js
Outdated
Show resolved
Hide resolved
@@ -55,6 +55,8 @@ const RuleBuilder = (props) => { | |||
const currentlySelectedRuleData = useSelector(getCurrentlySelectedRuleData); | |||
const currentlySelectedRuleConfig = useSelector(getCurrentlySelectedRuleConfig); | |||
|
|||
console.log("currentlySelectedRuleData", currentlySelectedRuleData); |
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.
Remove this log
...components/features/rules/RuleBuilder/Header/ActionButtons/CreateRuleButton/actions/index.js
Outdated
Show resolved
Hide resolved
app/src/components/misc/CodeEditor/EditorToast/editorToast.scss
Outdated
Show resolved
Hide resolved
app/src/components/misc/CodeEditor/EditorToast/EditorToastContainer.tsx
Outdated
Show resolved
Hide resolved
app/src/components/misc/CodeEditor/EditorToast/EditorToastContainer.tsx
Outdated
Show resolved
Hide resolved
const handleCodeFormattedFlag = () => { | ||
setIsCodeFormatted(true); | ||
setTimeout(() => { | ||
setIsCodeFormatted(false); | ||
}, 2000); | ||
}; |
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 function is duplicated inside renderCodeEditor
also.
const renderURLInput = () => { | ||
const renderCodeEditorForScriptAttributes = () => { |
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.
Can be very confusing.
renderCodeEditorForScriptAttributes
inside renderURLInput
.
Can we have a common code editor component which can be wrapper for CodeEditor
pass props accordingly for script
and url
type code editor.
This will also remove code duplication in both editor render functions
…q.scripthandler (#1400)
Schema changes
attributes
attribute to the script object (so inside the schema it is rule > pair > script.attributes)script.value
still only contains the raw code.Validations
During Editing
innerText
for html of script rule ofurl
typeOn Save:
document.onLoad
in any way, and loadtime isAfterPageLoad
: updates load time and shows editor toast, rule is not saved thoscript.value
contains html code, error toast is shownNew functionalities:
Code editor now have their own toasts
New UI: