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

Fix CSS Inlining when using embedded image using Laravel 10 or 9. #313

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

danhanly
Copy link
Contributor

Added the suggested changes as seen here: #311

AbstractMultipartPart is a parent of both of the existing conditions, so it shouldn't change the functionality.

Tests pass.

@DannyvdSluijs
Copy link
Collaborator

Quoting from #311

When you were working on this, did you get this to work with Laravel 10 by making your suggested change?

@danhanly where the changes tested and could it cause issues with older versions of Laravel Framework?

@kirkbushell
Copy link

I've made this change locally and it changes nothing - I still have tailwind css class names in the HTML.

@DannyvdSluijs
Copy link
Collaborator

Hi @kirkbushell

Thanks for your feedback. Could you elaborate a bit more on which version you used, against which version of Laravel etc. Or maybe even share your repo, gist or just plain bits of code (so called reproduction steps) or other things you might have found during your testing, in order to move this PR forward.

@kirkbushell
Copy link

Hi @kirkbushell

Thanks for your feedback. Could you elaborate a bit more on which version you used, against which version of Laravel etc. Or maybe even share your repo, gist or just plain bits of code (so called reproduction steps) or other things you might have found during your testing, in order to move this PR forward.

Can see my issue here where I found the problem: #201

I think it needs to be documented on the project.

@danhanly
Copy link
Contributor Author

danhanly commented Sep 13, 2023

Quoting from #311
where the changes tested and could it cause issues with older versions of Laravel Framework?

I've not tested it as I need it to support Laravel 10 before I'm able to consider integrating this into my project.

However, as I mentioned, the original condition compares against two classes AlternativePart and MixedPart both of which inherit from AbstractMultipartPart so there's no reason why it wouldn't work.

The existing test suite is comfortable with the change and the functionality continues regardless of it.

That being said, I know often this assurance isn't enough to warrant merging, since it could break compatibility. At the moment, I only have a Laravel 10 project in which to test this, so I can't verify it won't break previous versions, I'd appreciate it if someone could help test this in a previous laravel version.

I'm not certain this solves the underlying problem, since I was just actioning the suggestion made in #311 but I'm nearly at the point in my project where this will be required, so I'll be able to give it a good spin soon enough (on L10).

@kirkbushell
Copy link

Quoting from #311
where the changes tested and could it cause issues with older versions of Laravel Framework?

I've not tested it as I need it to support Laravel 10 before I'm able to consider integrating this into my project.

However, as I mentioned, the original condition compares against two classes AlternativePart and MixedPart both of which inherit from AbstractMultipartPart so there's no reason why it wouldn't work.

The existing test suite is comfortable with the change and the functionality continues regardless of it.

That being said, I know often this assurance isn't enough to warrant merging, since it could break compatibility. At the moment, I only have a Laravel 10 project in which to test this, so I can't verify it won't break previous versions, I'd appreciate it if someone could help test this in a previous laravel version.

I'm not certain this solves the underlying problem, since I was just actioning the suggestion made in #311 but I'm nearly at the point in my project where this will be required, so I'll be able to give it a good spin soon enough (on L10).

It's not a laravel 10 issue, as I got it working on Laravel 10. My problem was that the assets weren't built and therefore were not visible to css-inliner, as I am using vite.

@DannyvdSluijs DannyvdSluijs changed the title Include suggested changes for Laravel 10 Fix CSS Inlining when using embedded image using Laravel 10 or 9. Sep 18, 2023
Copy link
Collaborator

@DannyvdSluijs DannyvdSluijs left a comment

Choose a reason for hiding this comment

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

Successfully tested the changes using the following matrix:

  • Laravel 10 with Content-Type: text/html; charset=utf-8
  • Laravel 10 with Content-Type: multipart/related;
  • Laravel 9 with Content-Type: text/html; charset=utf-8
  • Laravel 9 with Content-Type: multipart/related;

@DannyvdSluijs DannyvdSluijs merged commit 47c41ed into fedeisas:master Sep 18, 2023
7 checks passed
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