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

Enhanced MJML Compilation and README Update #5

Closed
wants to merge 2 commits into from

Conversation

leknoppix
Copy link
Contributor

I hope this message finds you well. Firstly, I want to express my gratitude for the fantastic work you've put into maintaining this package. It has been an invaluable tool for my projects.

I've made some enhancements to the MJML compilation process that I believe will be beneficial for the community. Specifically, I've added the option to skip the compilation of text versions in MJML, resulting in separate Blade files for both HTML and text versions after compilation.

Previously, with files like mjml/index.mjml for the HTML version and text/index.php for the text version, the compiled output would be mjml/index.blade.php for HTML and text/index.blade.php for text. This modification facilitates the inclusion of both versions in Laravel mail delivery, supporting multipart functionality.

In addition to this, I plan to push the corresponding unit test later this evening to ensure the robustness of the changes.

I've also made an adjustment to the README to document the new "exclude" option for users who may want to exclude specific files during the compilation process.

Thank you for considering this pull request. I'm looking forward to your feedback, and I'm happy to address any questions or concerns.

Best regards,

@innocenzi innocenzi marked this pull request as draft January 29, 2024 10:34
@leknoppix
Copy link
Contributor Author

Hello,

I've been trying for hours to get my unit test to work, but with no success.
Here is what I propose as a test:

test('it can compile PHP and MJML files', async () => {
	expect(fs.existsSync(output)).toBe(false)

	await build({
		root: fixtures,
		logLevel: 'silent',
		plugins: [
			mjml({
				log: false,
				extension: '.html',
				input: path.resolve(fixtures, 'with-php'),
				output,
			}),
		],
	})

	expect(fs.existsSync(path.resolve(output, 'mail', 'mail.html'))).toBe(true)
	expect(fs.existsSync(path.resolve(output, 'mail', 'structure.html'))).toBe(true)
})

The idea is to compile two files present in the "with-php" folder to obtain the correct final structure.

I tested locally without running the tests, and it works. So, I get a "blade.php" file that corresponds to the MJML structure, and another ".blade.php" file that corresponds to the TEXT structure of the same email.

I hope I have been clear.

Good evening, and sorry, I really struggle with JavaScript unit tests.

@innocenzi
Copy link
Owner

Hey there @leknoppix, I'm trying to understand the final goal of the PR. The lengthy PR message doesn't really help me get it unfortunately. 😓

Is your intent to improve the support of (not) compiling "plain text" emails, that would happen to be in the same directory as your mjml templates?

Could you please use a tool like https://tree.nathanfriend.io/ to show me the file structure you'd like to have before compilation, and the one you'd like to have after compilation?

Thank you!

@leknoppix
Copy link
Contributor Author

leknoppix commented Jan 30, 2024

Hello and thank you for your response.

The idea behind my Pull Request (PR) is quite simple.

Here is an example structure:
[link to example structure](https://tree.nathanfriend.io/?s=(%27options!(%27fancy3~fullPath!false~tr6ingSlash3~rootDot3)~5(%275%27mj4ht4-mj47-php8em6*ht42*702%27)~version!%271%27)*80-0m6.0%20%202-blade.php3!true4ml*5source!6ail7text*8%5Cn%0187654320-*

In the end, this allows for preparing the text structure, just like for MJML files, without any HTML tags.

The compilation process, for MJML files, involves compiling them (initial action of this module). If PHP files are detected, it simply involves copying and pasting them to the intended destination.

@innocenzi
Copy link
Owner

Your link isn't working!

If PHP files are detected, it simply involves copying and pasting them to the intended destination.

Is this required because the target directory is .gitignored? I would think you'd have a resources/views/emails/html and resources/views/emails/plain directory in that situation, and the output option of this plugin would be set to resources/views/emails/html.

I'm not against adding the feature, but I would like to ensure there is no viable alternative before

@leknoppix
Copy link
Contributor Author

leknoppix commented Jan 31, 2024

I've found an alternative approach that doesn't involve using your module. I'll be exploring it through the https://www.npmjs.com/package/vite-plugin-static-copy module. Thus, your module will only serve for the MJML to HTML conversion, and that's it.

The vite-plugin-static-copy module will only be used for copying the plain versions.

viteStaticCopy({
            targets: [
                {
                    src: 'resources/mjml/email/text',
                    dest: '../../resources/views/templatemail/email/text',
                },
            ]
        }),

If this works for you, please close my pull request, as it might be more logical to separate these two actions.

@innocenzi
Copy link
Owner

innocenzi commented Jan 31, 2024

I also think it's better to separate these functionalities, but in practice I haven't implemented plain and html emails at the same time so I'm not sure about the DX

I'll close the PR for now, but if you feel there is too much friction with the additional I'm open to still adding the functionality in this plugin 👍

@innocenzi innocenzi closed this Jan 31, 2024
@leknoppix
Copy link
Contributor Author

I also think it's better to separate these functionalities, but in practice I haven't implemented plain and html emails at the same time so I'm not sure about the DX

I'll close the PR for now, but if you feel there is too much friction with the additional I'm open to still adding the functionality in this plugin 👍

My solution works and it doesn't overload the build execution.
It might be interesting to simply add, in the readme, that for the plaintext version, make a folder with the files in *.blade.php and use the module previously mentioned, no need to modify your module for the plaintext

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.

2 participants