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

Refactor: Update Email template settings in the form builder #6890

Merged
merged 33 commits into from
Aug 31, 2023

Conversation

JoshuaHungDinh
Copy link
Contributor

@JoshuaHungDinh JoshuaHungDinh commented Aug 18, 2023

Description

Reference: slack conversation about the cancel option: https://lw.slack.com/archives/C04SLRDD9CK/p1692899169132059

This PR includes several changes revolving around the Email Notification Settings. The major change involves the option that allows users to either set the email template fields or cancel the field values.

This was done including by including local state within the email template component to capture all the field changes {emailTemplateFieldValues } and dispatching the actual Form Setting changes when the Set button is clicked. The original option config is used to update the local state when the Cancel button is clicked to revert the field values.

Both action buttons "Set and close" & "Cancel" were moved down from the parent container to the EmailTemplateSettings component. I Initially I had looked into raising the state up to the parent in order the Submit the changes. Doing so required moving most the config logic from within the EmailTemplateSettings to the parent also. This made it harder to follow and maintain. Because of this I opted to move the action buttons down and absolutely position them to achieve an identical layout.

The changes reflect the QA tasks below, along with style, typescript type & text changes.

QA Tasks completed within PR:

Visuals

Screen Shot 2023-08-24 at 1 00 20 PM Screen Shot 2023-08-24 at 1 00 13 PM

Testing Instructions

Please note this PR no longer contains logic to cancel the selected email options. As the logic was included than removed in favor of a possible undo feature with gutenberg.

Please test that the email fields are working as they previously did.
Verify no fatal errors occur on email preview.
Verify default email address is populated in the send test email input.

https://www.loom.com/share/580ee957a673491ea21e01a127ed74f4?sid=93eed98b-fc59-4ed3-836f-aa7064892085

Pre-review Checklist

  • Relevant @unreleased tags included in DocBlocks
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@JoshuaHungDinh JoshuaHungDinh changed the title Refactor: Add ability to "save" or "close" Email Notification fields in the form builder Refactor: Add support to "save" or "close" Email Notification fields in the form builder Aug 22, 2023
@JoshuaHungDinh JoshuaHungDinh marked this pull request as ready for review August 24, 2023 00:37
@JoshuaHungDinh JoshuaHungDinh changed the title Refactor: Add support to "save" or "close" Email Notification fields in the form builder Refactor: Add option to "Set" or "Cancel" Email Template settings in the form builder Aug 24, 2023
Copy link
Member

@kjohnson kjohnson left a comment

Choose a reason for hiding this comment

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

I don't recall seeing the feature to "cancel" changes in the related tasks. Did we decide on this somewhere? I remember voting against adding a "cancel" feature, as this would be a major break from convention of other settings.

@JoshuaHungDinh
Copy link
Contributor Author

JoshuaHungDinh commented Aug 24, 2023

@kjohnson, At one point the task was set and close with cancel & high priority. I had spoken to @jdghinson and briefly with @pauloiankoski about it during the cycle. Im not sure when it updated to just close. We have been going back and forth mainly working from the Design doc. Nonetheless, we can remove it if it's that unconventional. Personally I don't mind it, but Im biased haha. Would changing the context to reset and keeping it open be viable?

Reference: https://lw.slack.com/archives/C04SLRDD9CK/p1692899169132059

@JoshuaHungDinh JoshuaHungDinh changed the title Refactor: Add option to "Set" or "Cancel" Email Template settings in the form builder Refactor: Update Email template settings in the form builder Aug 24, 2023
@JoshuaHungDinh
Copy link
Contributor Author

@kjohnson Cancel has been removed, I brought the button back up to the parent component and updated its text.

@kjohnson
Copy link
Member

kjohnson commented Aug 25, 2023

@JoshuaHungDinh I'm seeing the "Back to template settings" button below the preview, but shouldn't it be in the header?

image

Copy link
Member

@kjohnson kjohnson left a comment

Choose a reason for hiding this comment

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

The "Back to template settings" button should be in the modal header, but it is showing below the rendered email preview.

@JoshuaHungDinh
Copy link
Contributor Author

Ah very odd. It should be in the header. Let me take a look.

@JoshuaHungDinh
Copy link
Contributor Author

JoshuaHungDinh commented Aug 25, 2023

@kjohnson Looks like I had updated a className but not the style sheet. a27e7fa
Screen Shot 2023-08-25 at 12 07 06 PM

Copy link
Member

@kjohnson kjohnson left a comment

Choose a reason for hiding this comment

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

Thanks for updating that button position. To QA!

@JoshuaHungDinh
Copy link
Contributor Author

JoshuaHungDinh commented Aug 29, 2023

@kjohnson, Im just leaving a comment here as a reference to some additional changes.

During QA they tested the send email test function. It seemed to be failing when P2P was installed & I was having some difficulties sending the html emails. I have expanded the scope of this PR to address this as well. Below is a screenshot of the error while P2P was installed.

Screen Shot 2023-08-29 at 4 37 19 PM

Resolved: 69bc2cc
Resolved: 69e8491 - I realize the file was formatted & the change may be hard to track down. The only change was a conditional check that the $variable is an array before calling array_key_exists($array_key_name, $variable); . It can be found on line 892. The function that includes the change is listed below:

function give_check_variable($variable, $conditional = '', $default = false, $array_key_name = '')
{
    // Get value from array if array key non empty.
    if (empty($array_key_name)) {
        switch ($conditional) {
            case 'isset_empty':
                $variable = (isset($variable) && ! empty($variable)) ? $variable : $default;
                break;

            case 'empty':
                $variable = ! empty($variable) ? $variable : $default;
                break;

            case 'null':
                $variable = ! is_null($variable) ? $variable : $default;
                break;

            default:
                $variable = isset($variable) ? $variable : $default;
        }
    } else {
        $isset = is_array($variable) && array_key_exists($array_key_name, $variable);

        switch ($conditional) {
            case 'isset_empty':
                $variable = ($isset && ! empty($variable[$array_key_name])) ? $variable[$array_key_name] : $default;
                break;

            case 'empty':
                $variable = ! empty($variable[$array_key_name]) ? $variable[$array_key_name] : $default;
                break;

            case 'null':
                $variable = $isset && ! is_null($variable[$array_key_name]) ? $variable[$array_key_name] : $default;
                break;

            default:
                $variable = $isset && isset($variable[$array_key_name]) ? $variable[$array_key_name] : $default;
        }
    }

    return $variable;
}

Copy link
Member

@rickalday rickalday left a comment

Choose a reason for hiding this comment

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

Passed manual QA tests

@JoshuaHungDinh JoshuaHungDinh merged commit 849eed6 into epic/givewp-3.0 Aug 31, 2023
20 checks passed
@JoshuaHungDinh JoshuaHungDinh deleted the feature/email-save-and-close branch August 31, 2023 18:15
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