Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the functionality and improves the robustness of the frontend components. It introduces new advanced field types for date/time, file uploads, and conversational forms, while also refining the core CSS processing and conditional logic systems. These changes aim to provide a more feature-rich and stable user experience across various form interactions. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new documentation file detailing external service usage for transparency. It also significantly refines the internal workings of CSS atomization and the conditional logic system, making them more robust and feature-rich. Furthermore, it lays the groundwork for conversational forms with new components and message handling. A large number of dependency-related files were added, indicating a comprehensive update to the project's build and development environment. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the project's frontend build processes and enhances the robustness and functionality of core features. It introduces a new documentation file for external service transparency and significantly refines the conditional logic system, making it more powerful and secure. The changes also reflect an update in how project dependencies are managed across various packages. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant number of new files, including new packages and features, alongside some refactoring. The most critical issue is the inclusion of node_modules directories in the repository. These contain hardcoded local paths that will break the build for other developers. They must be removed and added to .gitignore. Additionally, I've identified a security concern with the use of new Function() for parsing configurations, which could lead to XSS vulnerabilities, and a potential bug that could generate invalid CSS selectors. Please address these points to improve the codebase's security, stability, and maintainability.
| #!/bin/sh | ||
| basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')") | ||
|
|
||
| case `uname` in | ||
| *CYGWIN*) basedir=`cygpath -w "$basedir"`;; | ||
| esac | ||
|
|
||
| if [ -z "$NODE_PATH" ]; then | ||
| export NODE_PATH="/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/node_modules" | ||
| else | ||
| export NODE_PATH="/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/node_modules:$NODE_PATH" | ||
| fi | ||
| if [ -x "$basedir/node" ]; then | ||
| exec "$basedir/node" "$basedir/../../../../node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/c8.js" "$@" | ||
| else | ||
| exec node "$basedir/../../../../node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/c8.js" "$@" | ||
| fi |
There was a problem hiding this comment.
This file, and the entire node_modules directory, should not be committed to version control. It contains hardcoded absolute paths to a local development environment (e.g., /home/maruf/... on lines 9 and 11), which will cause the build to fail for other developers and in CI/CD pipelines. Please remove all node_modules directories from the repository and add node_modules/ to your .gitignore file. This applies to all node_modules directories and other generated dependency files included in this pull request.
| @@ -227,13 +226,33 @@ export const expressCalcFunc = (calcStr, cssVarDefinations = {}) => { | |||
| const val1 = Number(variableDefinedArr[0].replace(/[^\.\d\s]/g, '')) | |||
| const val2 = Number(variableDefinedArr[2].replace(/[^\.\d\s]/g, '')) | |||
| // eslint-disable-next-line no-eval | |||
There was a problem hiding this comment.
Using eval() is a significant security risk as it can execute arbitrary code. It's great that you've replaced it with a safer, custom calculate function. This improves both the security and maintainability of the code.
| this.#bitFlatPickrInstance = flatpickr | ||
| if (config?.advancedConfig) { | ||
| try { | ||
| this.#advanceConfig = (new Function(`return ${config.advancedConfig}`))() |
There was a problem hiding this comment.
The use of new Function() to parse config.advancedConfig is a potential security vulnerability, similar to using eval(). If the advancedConfig string can be influenced by a user, it could lead to Cross-Site Scripting (XSS) attacks. It's strongly recommended to use a safer method for parsing configurations, such as JSON.parse(). If the configuration needs to contain functions, consider refactoring the design to avoid executing arbitrary code from a string.
| if (!combinedCssRulesObj[validConcatSelectors.join(',')]) { | ||
| combinedCssRulesObj[validConcatSelectors.join(',')] = { [prop]: value } | ||
| } else { | ||
| combinedCssRulesObj[sameProValSelectors.join(',')][prop] = value | ||
| combinedCssRulesObj[validConcatSelectors.join(',')][prop] = value | ||
| } |
There was a problem hiding this comment.
If validConcatSelectors is an empty array, validConcatSelectors.join(',') will produce an empty string ''. This will result in creating a CSS rule with an empty selector ('' { ... }), which is invalid and might cause issues in some CSS parsers or browsers. You should add a check to ensure validConcatSelectors is not empty before joining and creating the rule.
if (validConcatSelectors.length > 0) {
if (!combinedCssRulesObj[validConcatSelectors.join(',')]) {
combinedCssRulesObj[validConcatSelectors.join(',')] = { [prop]: value }
} else {
combinedCssRulesObj[validConcatSelectors.join(',')][prop] = value
}
}
There was a problem hiding this comment.
Code Review
This pull request adds a significant number of generated files, build artifacts, and local dependencies (node_modules) to the repository. Committing these files is highly discouraged as it bloats the repository, can cause merge conflicts, and, most critically, includes files with hardcoded absolute paths from a local development environment (e.g., /home/maruf/...). This will break the project for other developers and in CI/CD pipelines. Please add node_modules/, dist/, *.log, and other build artifacts to your .gitignore file and remove these files from the pull request. Additionally, there are a few areas for improvement in the source code, including a security vulnerability related to the use of new Function(), and some opportunities for code clarification and bug prevention.
| export NODE_PATH="/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/node_modules" | ||
| else | ||
| export NODE_PATH="/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/node_modules:$NODE_PATH" |
There was a problem hiding this comment.
This file contains hardcoded absolute paths to a local directory (/home/maruf/...). This will cause the script to fail in any other environment. This file, along with the entire node_modules directory, is a local dependency and should not be committed to version control. Please ensure node_modules is listed in your .gitignore file. This comment applies to all similar executable files found in .bin directories within this pull request.
| this.#bitFlatPickrInstance = flatpickr | ||
| if (config?.advancedConfig) { | ||
| try { | ||
| this.#advanceConfig = (new Function(`return ${config.advancedConfig}`))() |
There was a problem hiding this comment.
Using new Function() to parse advancedConfig is a security risk, as it can execute arbitrary JavaScript code. If the advancedConfig string comes from an untrusted source (like user input), this could lead to a code injection vulnerability. It's highly recommended to use a safer method for parsing configurations, such as JSON.parse(). If the configuration needs to contain functions, consider refactoring to pass them in a more secure way.
| if (!combinedCssRulesObj[validConcatSelectors.join(',')]) { | ||
| combinedCssRulesObj[validConcatSelectors.join(',')] = { [prop]: value } | ||
| } else { | ||
| combinedCssRulesObj[sameProValSelectors.join(',')][prop] = value | ||
| combinedCssRulesObj[validConcatSelectors.join(',')][prop] = value | ||
| } |
There was a problem hiding this comment.
If validConcatSelectors is an empty array, validConcatSelectors.join(',') will result in an empty string ''. This will create a CSS rule with an empty selector, which is likely not intended and could lead to unexpected behavior. It's better to guard this block to only execute if there are valid selectors to combine.
if (validConcatSelectors.length > 0) {
const combinedSelector = validConcatSelectors.join(',');
if (!combinedCssRulesObj[combinedSelector]) {
combinedCssRulesObj[combinedSelector] = { [prop]: value };
} else {
combinedCssRulesObj[combinedSelector][prop] = value;
}
}| if (value === undefined || value === null) value = '' | ||
| let newValue = value | ||
| .trim() | ||
| ?.trim() |
There was a problem hiding this comment.
The optional chaining (?.) on .trim() is redundant here. The preceding line if (value === undefined || value === null) value = '' ensures that value is always a string, so it will always have the trim method. Removing the optional chaining will make the code slightly cleaner.
| ?.trim() | |
| .trim() |
| @@ -1,6 +1,5 @@ | |||
| /* eslint-disable no-useless-escape */ | |||
| import colorMinify from './colorMinify' | |||
| import deepCopy from './deepCopy' | |||
There was a problem hiding this comment.
The deepCopy import is no longer used in this file and can be removed.
| // this.#document = null | ||
| // this.#dateTimeInputHidden = null |
There was a problem hiding this comment.
This commented-out code should be removed. Leaving dead code in the repository can cause confusion and reduce maintainability.
|
|
||
| #addEvent(element, eventType, eventAction) { | ||
| element.addEventListener(eventType, eventAction) | ||
| // this.#allEventListeners.push({ element, eventType, eventAction }) |
There was a problem hiding this comment.
This line is commented out. Dead code should be removed to improve code clarity and maintainability.
| elm?.setAttribute?.(name, value) | ||
| } | ||
|
|
||
| #select(selector) { return this.#fieldUploadWrapper.querySelector(selector) || console.error('selector not found', selector) } |
There was a problem hiding this comment.
Using the logical OR operator (||) for side effects like logging an error can be confusing and is not a common pattern. It would be more readable and explicit to use a standard if statement to check for the element and log an error if it's not found.
#select(selector) {
const element = this.#fieldUploadWrapper.querySelector(selector)
if (!element) {
console.error('selector not found', selector)
}
return element
}| return function (...args) { | ||
| clearTimeout(timeoutId) | ||
| timeoutId = setTimeout(() => { | ||
| func.apply(this, args) | ||
| }, delay) | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of handleFormSaveDraft calls saveFormProgress directly on every event. This can lead to excessive calls, especially on frequent events like on_input. It's better to debounce this function to improve performance and avoid overwhelming the backend.
function debounce(func, delay) {
let timeoutId
return function (...args) {
clearTimeout(timeoutId)
timeoutId = setTimeout(() => {
func.apply(this, args)
}, delay)
}
}
const debouncedSaveFormProgress = debounce((contentId) => typeof saveFormProgress !== 'undefined' && saveFormProgress(contentId), 2500)
const handleFormSaveDraft = (actionDetail, props) => {
debouncedSaveFormProgress(props.contentId)
}
There was a problem hiding this comment.
Code Review
This pull request introduces several new features and fixes, particularly around conditional logic and new field types. The refactoring to move logic into helper functions and make the code more robust is a good improvement.
However, there are a few critical issues that need to be addressed:
-
Committed
node_modulesand build artifacts: A large number of generated files fromnode_modules,dist, and.turbodirectories have been included in this PR. These files are user-specific, contain absolute paths, and should not be part of the version control system. They will cause the project to break for other developers and unnecessarily bloat the repository. Please remove these directories from the PR and add them to your.gitignorefile. -
Security Concern: The use of
new Function()in the new advanced datetime field poses a potential XSS security risk. It's highly recommended to use a safer method likeJSON.parse()for handling configurations.
I've left specific comments on the code with detailed feedback and suggestions for the issues I've found.
| #!/bin/sh | ||
| basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')") | ||
|
|
||
| case `uname` in | ||
| *CYGWIN*) basedir=`cygpath -w "$basedir"`;; | ||
| esac | ||
|
|
||
| if [ -z "$NODE_PATH" ]; then | ||
| export NODE_PATH="/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/node_modules" | ||
| else | ||
| export NODE_PATH="/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules/c8/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/c8@7.14.0/node_modules:/home/maruf/Local Sites/bit-formorg/app/public/wp-content/plugins/bitform/frontend-dev/node_modules/.pnpm/node_modules:$NODE_PATH" | ||
| fi | ||
| if [ -x "$basedir/node" ]; then | ||
| exec "$basedir/node" "$basedir/../../../../node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/c8.js" "$@" | ||
| else | ||
| exec node "$basedir/../../../../node_modules/.pnpm/c8@7.14.0/node_modules/c8/bin/c8.js" "$@" | ||
| fi |
There was a problem hiding this comment.
This file, along with many others in this pull request, appears to be part of the node_modules directory and contains hardcoded absolute paths specific to a user's local machine (e.g., /home/maruf/...).
Checking node_modules and other generated files (like those in dist/ and .turbo/) into version control is a critical issue because:
- It breaks the build for other developers whose local paths do not match.
- It unnecessarily bloats the repository size.
- It can lead to hard-to-debug dependency conflicts.
The entire node_modules directory, as well as other build artifact directories, should be added to your .gitignore file to prevent these files from being committed. Please remove all such files from this pull request.
| if (!combinedCssRulesObj[validConcatSelectors.join(',')]) { | ||
| combinedCssRulesObj[validConcatSelectors.join(',')] = { [prop]: value } | ||
| } else { | ||
| combinedCssRulesObj[sameProValSelectors.join(',')][prop] = value | ||
| combinedCssRulesObj[validConcatSelectors.join(',')][prop] = value | ||
| } |
There was a problem hiding this comment.
If validConcatSelectors is an empty array (which can happen if all selectors with the same property-value pair have vendor prefixes), validConcatSelectors.join(',') will result in an empty string. This will create a CSS rule with an empty selector ('' { ... }), which is invalid and can cause issues in CSS parsing.
You should add a check to ensure validConcatSelectors has elements before creating the combined rule.
if (validConcatSelectors.length > 0) {
if (!combinedCssRulesObj[validConcatSelectors.join(',')]) {
combinedCssRulesObj[validConcatSelectors.join(',')] = { [prop]: value }
} else {
combinedCssRulesObj[validConcatSelectors.join(',')][prop] = value
}
}| this.#bitFlatPickrInstance = flatpickr | ||
| if (config?.advancedConfig) { | ||
| try { | ||
| this.#advanceConfig = (new Function(`return ${config.advancedConfig}`))() |
There was a problem hiding this comment.
Using new Function() to parse config.advancedConfig is a security risk, similar to using eval(). If advancedConfig can be influenced by a user, it could lead to Cross-Site Scripting (XSS) vulnerabilities. It is strongly recommended to use a safer method for parsing this configuration, such as JSON.parse(). If the configuration needs to contain functions, consider a safer approach like registering functions and referencing them by name, rather than executing arbitrary code from a string.
| | **Autonami** | Marketing automation | Form field values | [Autonami](https://autonami.io/) | See service website | | ||
| | **Groundhogg** | Marketing automation | Form field values | [Groundhogg](https://www.groundhogg.io/) | See service website | | ||
| | **Encharge** | Email marketing | Form field values | [Encharge Terms](https://encharge.io/terms/) | [Encharge Privacy](https://encharge.io/privacy-policy/) | | ||
| | **SendFox** | Email marketing | Form field values | [SendFox](https://sendfox.com/) | See service website | |
There was a problem hiding this comment.
For better transparency and user convenience, it would be helpful to provide direct links to the Terms of Service and Privacy Policy for services where it currently says 'See service website'. I've found the links for a few of them.
| | **Autonami** | Marketing automation | Form field values | [Autonami](https://autonami.io/) | See service website | | |
| | **Groundhogg** | Marketing automation | Form field values | [Groundhogg](https://www.groundhogg.io/) | See service website | | |
| | **Encharge** | Email marketing | Form field values | [Encharge Terms](https://encharge.io/terms/) | [Encharge Privacy](https://encharge.io/privacy-policy/) | | |
| | **SendFox** | Email marketing | Form field values | [SendFox](https://sendfox.com/) | See service website | | |
| | **Autonami** | Marketing automation | Form field values | [Autonami Terms](https://autonami.com/terms-and-conditions/) | [Autonami Privacy](https://autonami.com/privacy-policy/) | | |
| | **Groundhogg** | Marketing automation | Form field values | [Groundhogg Terms](https://www.groundhogg.io/terms-and-conditions/) | [Groundhogg Privacy](https://www.groundhogg.io/privacy-policy/) | | |
| | **Encharge** | Email marketing | Form field values | [Encharge Terms](https://encharge.io/terms/) | [Encharge Privacy](https://encharge.io/privacy-policy/) | | |
| | **SendFox** | Email marketing | Form field values | [SendFox Terms](https://sendfox.com/terms) | [SendFox Privacy](https://sendfox.com/privacy) | |
| | **Integrately** | Workflow automation | Form field values | [Integrately](https://integrately.com/) | See service website | | ||
| | **Pabbly** | Workflow automation | Form field values | [Pabbly](https://www.pabbly.com/terms) | See service website | |
There was a problem hiding this comment.
It would be more helpful for users if direct links to the privacy policies were provided here instead of 'See service website'.
| | **Integrately** | Workflow automation | Form field values | [Integrately](https://integrately.com/) | See service website | | |
| | **Pabbly** | Workflow automation | Form field values | [Pabbly](https://www.pabbly.com/terms) | See service website | | |
| | **Integrately** | Workflow automation | Form field values | [Integrately Terms](https://integrately.com/terms) | [Integrately Privacy](https://integrately.com/privacy-policy) | | |
| | **Pabbly** | Workflow automation | Form field values | [Pabbly Terms](https://www.pabbly.com/terms) | [Pabbly Privacy](https://www.pabbly.com/privacy-policy/) | |
| if (strArr.length === 2) { | ||
| return `${strArr[0]} ${strArr[1]}${hasImportant}` | ||
| return `${strArr[0]} ${strArr[1]}${hasImportant} ` | ||
| } | ||
| if (strArr.length === 3 && strArr[0] === strArr[2]) { | ||
| return `${strArr[0]} ${strArr[1]}${hasImportant}` | ||
| return `${strArr[0]} ${strArr[1]}${hasImportant} ` | ||
| } | ||
| if (strArr.length === 4 && strArr.every((val, i, arr) => val === arr[0])) { | ||
| return strArr[0] + hasImportant | ||
| } | ||
| if (strArr.length === 4 && strArr[0] === strArr[2] && strArr[1] === strArr[3]) { | ||
| return `${strArr[0]} ${strArr[1]}${hasImportant}` | ||
| return `${strArr[0]} ${strArr[1]}${hasImportant} ` | ||
| } |
There was a problem hiding this comment.
Adding a trailing space here seems incorrect and could cause issues with CSS parsing. The goal is likely to correctly format !important, but this change doesn't achieve that and may introduce subtle bugs.
A more robust way to handle !important would be to include the leading space when defining hasImportant, and then append it without any extra spaces.
For example:
const hasImportant = str.includes(' !important') ? ' !important' : '';
const trimmedStr = str.replace(/ !important/g, '').trim();
// ...
// then in return statements:
return `${strArr[0]} ${strArr[1]}${hasImportant}`;This would correctly format values like 10px 20px !important.
| elm?.setAttribute?.(name, value) | ||
| } | ||
|
|
||
| #select(selector) { return this.#fieldUploadWrapper.querySelector(selector) || console.error('selector not found', selector) } |
There was a problem hiding this comment.
This method logs an error but returns undefined when an element is not found. This can lead to subsequent TypeError: Cannot read properties of undefined if the caller doesn't explicitly check the return value, making debugging harder.
It would be more robust to just return the result of querySelector (null if not found) and let the caller handle it, possibly using optional chaining (?.). The error logging is helpful, but the return value is what causes the downstream issue.
#select(selector) {
const element = this.#fieldUploadWrapper.querySelector(selector)
if (!element) {
console.error('selector not found', selector)
}
return element
}1 participant
No description provided.