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

Feature/greenhouse fix #256

Merged
merged 14 commits into from
Sep 7, 2022
Merged

Feature/greenhouse fix #256

merged 14 commits into from
Sep 7, 2022

Conversation

iruzevic
Copy link
Member

@iruzevic iruzevic commented Sep 2, 2022

Description

Added

  • custom form params are now in one place and used as a enum for PHP and JS.
  • server errors will no longer produce fatal error on the form but will output the msg to the user, and the msg is also translatable.
  • option to remove all unnecessary custom params that are set on the form before the final integration post so we don't send unnecessary stuff.
  • new admin setting sidebar title for grouping the sections
  • new troubleshooting section that contains debugging options: skip validation, form reset on submit, output log.
  • new fallback email fields for all integrations, this will send an email with all details if there is and issue with integration.

Changed

  • all JS global variables for frontend and backend are now using the same name.
  • internal custom field for actions is now called es-form-action.
  • filter for setting http request from httpRequestArgs to httpRequestTimeout because it is used only to set timeout.
  • Greenhouse integration from wp_remote_post to regular Curl because of the issues while sending the attachments. You are now no longer limited on the amount of memory on your server to send files.
  • Form will now throw and error if form ID or type is missing in the request.
  • all remote requests now output via helper for easier and more predictable output.

Fixed

  • all text domains that were wrong changed from eightshift-form to eightshift-forms.
  • Active campaign body was set wrong and was not working.
  • Active campaign setting info copy for setting api key and url.
  • customSuccess label is now translatable from settings.
  • validator will now skip input type hidden because there is no need for that.

Removed

  • ES_DEVELOP_MODE_SKIP_VALIDATION because it is used from admin now.
  • ES_LOG_MODE because it is used from admin now.

Screenshots

Screenshot 2022-09-05 at 15 41 24
Screenshot 2022-09-05 at 15 42 18
Screenshot 2022-09-02 at 15 13 09
Screenshot 2022-09-02 at 15 13 13

@iruzevic iruzevic added bug Something isn't working improvement Small improvement fixes, either readability or performance improvements integrations Issues with integrations labels Sep 2, 2022
@iruzevic iruzevic requested a review from a team September 2, 2022 06:59
@iruzevic iruzevic self-assigned this Sep 2, 2022
Copy link
Contributor

@iobrado iobrado left a comment

Choose a reason for hiding this comment

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

Nice job! Found few typos and added suggestions to fix them 🙂

src/Enqueue/Blocks/EnqueueBlocks.php Outdated Show resolved Hide resolved
src/Integrations/Hubspot/HubspotClient.php Outdated Show resolved Hide resolved
src/Integrations/Mailchimp/MailchimpClient.php Outdated Show resolved Hide resolved
src/Integrations/Mailerlite/MailerliteClient.php Outdated Show resolved Hide resolved
src/Mailer/Mailer.php Outdated Show resolved Hide resolved
src/Rest/Routes/FormSettingsSubmitRoute.php Outdated Show resolved Hide resolved
src/Rest/Routes/FormSettingsSubmitRoute.php Outdated Show resolved Hide resolved
src/Rest/Routes/FormSettingsSubmitRoute.php Outdated Show resolved Hide resolved
src/Rest/Routes/FormSubmitCustomRoute.php Outdated Show resolved Hide resolved
Copy link
Contributor

@goranalkovic-infinum goranalkovic-infinum left a comment

Choose a reason for hiding this comment

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

Left minor comments, awesome work!

src/Blocks/components/form/assets-admin/index.js Outdated Show resolved Hide resolved
src/Hooks/FiltersGeneral.md Outdated Show resolved Hide resolved
src/Hooks/FiltersGeneral.md Outdated Show resolved Hide resolved
src/Hooks/FiltersGeneral.md Outdated Show resolved Hide resolved
src/Integrations/ActiveCampaign/SettingsActiveCampaign.php Outdated Show resolved Hide resolved
src/Integrations/ActiveCampaign/SettingsActiveCampaign.php Outdated Show resolved Hide resolved
src/Rest/Routes/CacheDeleteRoute.php Outdated Show resolved Hide resolved
src/Rest/Routes/CacheDeleteRoute.php Outdated Show resolved Hide resolved
src/Rest/Routes/CacheDeleteRoute.php Outdated Show resolved Hide resolved
src/Rest/Routes/CacheDeleteRoute.php Outdated Show resolved Hide resolved
Co-authored-by: Goran Alković <[email protected]>
Co-authored-by: Igor Obradović <[email protected]>
Copy link
Collaborator

@dingo-d dingo-d left a comment

Choose a reason for hiding this comment

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

Left some comments.

src/Hooks/FiltersTracking.md Outdated Show resolved Hide resolved
src/Integrations/Greenhouse/GreenhouseClient.php Outdated Show resolved Hide resolved
src/Integrations/Greenhouse/GreenhouseClient.php Outdated Show resolved Hide resolved
src/Integrations/Greenhouse/GreenhouseClient.php Outdated Show resolved Hide resolved
Copy link
Contributor

@volfkarlo volfkarlo left a comment

Choose a reason for hiding this comment

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

A few micro comments.
👏

src/Blocks/components/form/assets/form.js Show resolved Hide resolved
src/Integrations/Goodbits/GoodbitsClient.php Outdated Show resolved Hide resolved
src/Integrations/Hubspot/HubspotClient.php Outdated Show resolved Hide resolved
iruzevic and others added 5 commits September 2, 2022 15:22
…se-fix

* feature/troubleshooting-features:
  adding separators to sidebar menu
  conveting ali api responses to usage of helper
  adding fallback email for integrations

# Conflicts:
#	src/Enqueue/Blocks/EnqueueBlocks.php
Copy link
Collaborator

@dingo-d dingo-d left a comment

Choose a reason for hiding this comment

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

Left several comments on improvements. Some parts I've omitted adding a comment if the same thing is happening in multiple places.

src/AdminMenus/FormSettingsAdminSubMenu.php Outdated Show resolved Hide resolved
src/Enqueue/Blocks/EnqueueBlocks.php Outdated Show resolved Hide resolved
src/Geolocation/Geolocation.php Outdated Show resolved Hide resolved
src/Geolocation/Geolocation.php Outdated Show resolved Hide resolved
src/Geolocation/Geolocation.php Outdated Show resolved Hide resolved
*
* @var MailerInterface
*/
public $mailer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this public?

@iruzevic iruzevic requested a review from dingo-d September 6, 2022 07:21
Copy link
Contributor

@volfkarlo volfkarlo left a comment

Choose a reason for hiding this comment

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

Micro comment, no need to correct 😄

👏

Comment on lines 53 to 54
<?php foreach ($innerItems as $item) { ?>
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for closing and opening PHP tags 🙂

@iruzevic iruzevic merged commit 1076cfb into develop Sep 7, 2022
@iruzevic iruzevic deleted the feature/greenhouse-fix branch September 7, 2022 06:37
iruzevic added a commit that referenced this pull request Sep 7, 2022
* develop:
  Feature/greenhouse fix (#256)

# Conflicts:
#	src/Geolocation/Geolocation.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working improvement Small improvement fixes, either readability or performance improvements integrations Issues with integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify the code Issue with JS loading and localized scripts
6 participants