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

EVF-580 Feature - Custom CSS and JS for form #1123

Merged
merged 21 commits into from
Jul 17, 2023
Merged

Conversation

salimshrestha98
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

This PR introduces a feature to add custom styles( CSS ) and scripts( JS ) to the frontend page where the form is loaded.

How to test the changes in this Pull Request:

  1. Make sure the newly added Custom CSS and JS setting menu is added in form builder settings.
  2. Verify all the features are working as expected.
  3. Try enabling/disabling the css and js. Try different styling and scripts.

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (modification of the currently available functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully ran tests with your changes locally?
  • Have you updated the documentation accordingly?

Changelog entry

EVF-580 Feature - Custom CSS and JS for form

Copy link
Contributor

@coderkoala coderkoala left a comment

Choose a reason for hiding this comment

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

Apologies for barging in, but this PR could use more streamlining: kindly use a package manager with a bundler to keep track of JS dependencies, especially something as big as ace. Since evf already uses npm with either webpack or grunt(afaik), bundling should be a breeze.

Has the EVF team worked over how they'll tackle how large the plugin payload will be post-deployment? This one is a bit concerning : It's not like the feature is unwelcome, but looking to use a CDN, perhaps a common repository that are already available could be a better approach as opposed to self-hosting(although some would definitely prefer that).

Again, fascinating approach WPEverest is taking with this feature, I'm still trying to wrap my head around why the team thinks this feature will be a good idea, and not a potential security risk - letting the frontend run code essentially arbritarily is a big no-no. It could end up encouraging common attack patterns such as XSS/ACE. This feature makes sense in a dev-oriented package, not something that'll be accessed by the average barely tech-saavy WordPress users. :)

Copy link
Member

@deepench deepench left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dipuchaudhary dipuchaudhary self-requested a review July 12, 2023 07:52
Copy link
Contributor

@dipuchaudhary dipuchaudhary left a comment

Choose a reason for hiding this comment

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

LGTM !!

@deepench deepench removed the request for review from iamprazol July 17, 2023 05:59
@deepench deepench merged commit a4e42a2 into develop Jul 17, 2023
@deepench deepench deleted the EVF-580-custom-css-js branch July 17, 2023 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants