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

Automation template listing [MAILPOET-4533] #4362

Merged
merged 16 commits into from
Sep 15, 2022

Conversation

websupporter
Copy link
Contributor

@websupporter websupporter commented Sep 14, 2022

Description

This PR adds the template listing page. You can reach the page by clicking the "New automation" button.

Linked PRs

https://github.com/mailpoet/mailpoet-premium/pull/622

Linked tickets

MAILPOET-4533

'mailpoet-automation-templates',
[$this, 'automationTemplates']
);

Copy link
Contributor

Choose a reason for hiding this comment

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

When I'm on page=mailpoet-automation-templates, I don't see MailPoet menu expanded and the "Automations" menu highlighted. We seem to have some solution for this that is used here, for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Thank you! Addressed in ada0ed3

setShowModal: Dispatch<SetStateAction<boolean>>;
};

function FromScratchPremiumModal({
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, the modal renders differently than in other places.
Here:

Screen Shot 2022-09-14 at 10 27 40

Automation editor:

Screen Shot 2022-09-14 at 10 27 31

Is that some kind of an issue with how the modal handles styles, maybe? We need to be able to display this modal in the same way anywhere in the MailPoet admin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The automation listing as well as the templates page did enqueue wp-component styles. I removed this in d7140cf

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh! And the removal is safe? Because we do use @wordpress/components so we need to load their styles somehow. In the editor, we bundle them now so I guess we need to bundle them also anywhere we use them and also within the modal?

It feels like removing these lines could break some existing styles, if @wordpress/components are used...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the editor, we bundle them now

We do so also in mailpoet-plugin.scss, which is loaded throughout the screens: https://github.com/mailpoet/mailpoet/blob/trunk/mailpoet/assets/css/src/mailpoet-plugin.scss#L24

I removed them now in 9fe003a from the mailpoet-automation-editor.scss as they are not needed there.

What we witness is an update of the styles (PR), This update affects Gutenberg version>13.7 while in WordPress core the current Gutenberg version is 13.0.

So, with our current way, we divert from core and "enqueue" the latest styles (or the styles of the components version, we require). Not entirely sure, if that's the best way (thinking of 3rd parties potentially enqueuing wp-components)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, thanks for the explanation! Yeah, I guess at this point it's correct to "divert" in the styles because we're diverting also with the JS (bundling them instead of using @wordpress/dependency-extraction-webpack-plugin) which we may fix in the future. It would require supporting only few recent versions of WP, and testing the UI across them. For now, we picked the "easier" way and did what's done in the form editor.

max-width: 982px;
}

ul.mailpoet-templates {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should include the automation in the prefix, and maybe the same below for mailpoet-from-scratch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 2f42d2c

@JanJakes JanJakes assigned websupporter and unassigned JanJakes Sep 14, 2022
@@ -2,6 +2,7 @@

namespace MailPoet\Automation\Engine\Builder;

use _PHPStan_ae8980142\Nette\InvalidStateException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ups 😄 Your IDE did this, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Addressed in 17e2f97

setShowModal: Dispatch<SetStateAction<boolean>>;
};

function FromScratchPremiumModal({
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh! And the removal is safe? Because we do use @wordpress/components so we need to load their styles somehow. In the editor, we bundle them now so I guess we need to bundle them also anywhere we use them and also within the modal?

It feels like removing these lines could break some existing styles, if @wordpress/components are used...

@JanJakes JanJakes assigned websupporter and unassigned JanJakes Sep 14, 2022
@websupporter websupporter force-pushed the MAILPOET-4533-automation-template-listing branch from d7140cf to c3f152e Compare September 15, 2022 04:43
They are already imported in mailpoet-plugin.scss

[MAILPOET-4355]
@@ -1,4 +1,3 @@
@import '../../../node_modules/@wordpress/components/build-style/style';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

setShowModal: Dispatch<SetStateAction<boolean>>;
};

function FromScratchPremiumModal({
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, thanks for the explanation! Yeah, I guess at this point it's correct to "divert" in the styles because we're diverting also with the JS (bundling them instead of using @wordpress/dependency-extraction-webpack-plugin) which we may fix in the future. It would require supporting only few recent versions of WP, and testing the UI across them. For now, we picked the "easier" way and did what's done in the form editor.

@JanJakes JanJakes merged commit c9c48aa into trunk Sep 15, 2022
@JanJakes JanJakes deleted the MAILPOET-4533-automation-template-listing branch September 15, 2022 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants